On Fri,  9 Dec 2016 19:57:16 +0000
Daniel Stone <dani...@collabora.com> wrote:

> Rather than duplicating knowledge of pixel formats across several
> components, create a custom central repository.
> 
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> 
> Differential Revision: https://phabricator.freedesktop.org/D1511
> ---
>  libweston/pixel-formats.c | 398 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  libweston/pixel-formats.h | 112 +++++++++++++
>  2 files changed, 510 insertions(+)
>  create mode 100644 libweston/pixel-formats.c
>  create mode 100644 libweston/pixel-formats.h
> 
> diff --git a/libweston/pixel-formats.c b/libweston/pixel-formats.c
> new file mode 100644
> index 0000000..9c70e73
> --- /dev/null
> +++ b/libweston/pixel-formats.c
> @@ -0,0 +1,398 @@
> +/*
> + * Copyright © 2016 Collabora, Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Author: Daniel Stone <dani...@collabora.com>
> + */
> +
> +#include "config.h"
> +
> +#include <inttypes.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <drm/drm_fourcc.h>
> +
> +#include "helpers.h"
> +#include "wayland-util.h"
> +#include "pixel-formats.h"
> +
> +#include <EGL/egl.h>
> +#include <EGL/eglext.h>
> +#include <GLES2/gl2.h>
> +#include <GLES2/gl2ext.h>
> +
> +#include "weston-egl-ext.h"
> +
> +/**
> + * Table of DRM formats supported by Weston; RGB, ARGB and YUV formats are
> + * supported. Indexed/greyscale formats, and formats not containing complete
> + * colour channels, are not supported.
> + */
> +static const struct pixel_format_info pixel_format_table[] = {
> +     {
> +             .format = DRM_FORMAT_XRGB4444,
> +     },
> +     {
> +             .format = DRM_FORMAT_ARGB4444,
> +             .opaque_substitute = DRM_FORMAT_XRGB4444,
> +     },
> +     {
> +             .format = DRM_FORMAT_XBGR4444,
> +     },
> +     {
> +             .format = DRM_FORMAT_ABGR4444,
> +             .opaque_substitute = DRM_FORMAT_XBGR4444,
> +     },
> +     {
> +             .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'?

> +     },
> +     {
> +             .format = DRM_FORMAT_RGBA4444,
> +             .opaque_substitute = DRM_FORMAT_RGBX4444,
> +             .gl_format = GL_RGBA,
> +             .gl_type = GL_UNSIGNED_SHORT_4_4_4_4,
> +     },
> +     {
> +             .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,
> +     },
> +     {
> +             .format = DRM_FORMAT_XRGB1555,
> +             .depth = 15,
> +             .bpp = 16,
> +     },
> +     {
> +             .format = DRM_FORMAT_ARGB1555,
> +             .opaque_substitute = DRM_FORMAT_XRGB1555,
> +     },
> +     {
> +             .format = DRM_FORMAT_XBGR1555,
> +     },
> +     {
> +             .format = DRM_FORMAT_ABGR1555,
> +             .opaque_substitute = DRM_FORMAT_XBGR1555,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGBX5551,
> +             .gl_format = GL_RGBA,
> +             .gl_type = GL_UNSIGNED_SHORT_5_5_5_1,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGBA5551,
> +             .opaque_substitute = DRM_FORMAT_RGBX5551,
> +             .gl_format = GL_RGBA,
> +             .gl_type = GL_UNSIGNED_SHORT_5_5_5_1,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGRX5551,
> +             .gl_format = GL_BGRA_EXT,
> +             .gl_type = GL_UNSIGNED_SHORT_5_5_5_1,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGRA5551,
> +             .opaque_substitute = DRM_FORMAT_BGRX5551,
> +             .gl_format = GL_BGRA_EXT,
> +             .gl_type = GL_UNSIGNED_SHORT_5_5_5_1,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGB565,
> +             .depth = 16,
> +             .bpp = 16,
> +             .gl_type = GL_RGB,
> +             .gl_type = GL_UNSIGNED_SHORT_5_6_5,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGR565,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGB888,
> +     },
> +     {
> +             .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.

> +     },
> +     {
> +             .format = DRM_FORMAT_XRGB8888,
> +             .depth = 24,
> +             .bpp = 32,
> +             .gl_format = GL_BGRA_EXT,
> +             .gl_type = GL_UNSIGNED_BYTE,

GL info incorrect for big-endian.

> +     },
> +     {
> +             .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. :-)

> +     },
> +     {
> +             .format = DRM_FORMAT_XBGR8888,
> +             .gl_format = GL_RGBA,
> +             .gl_type = GL_UNSIGNED_BYTE,
> +     },
> +     {
> +             .format = DRM_FORMAT_ABGR8888,
> +             .opaque_substitute = DRM_FORMAT_XBGR8888,
> +             .gl_format = GL_RGBA,
> +             .gl_type = GL_UNSIGNED_BYTE,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGBX8888,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGBA8888,
> +             .opaque_substitute = DRM_FORMAT_RGBX8888,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGRX8888,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGRA8888,
> +             .opaque_substitute = DRM_FORMAT_BGRX8888,
> +     },
> +     {
> +             .format = DRM_FORMAT_XRGB2101010,
> +             .depth = 30,
> +             .bpp = 32,
> +     },
> +     {
> +             .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.

> +     },
> +     {
> +             .format = DRM_FORMAT_XBGR2101010,
> +             .gl_type = GL_RGB,
> +             .gl_format = GL_UNSIGNED_INT_2_10_10_10_REV_EXT,

I think this might be illegal, having only 3 elements in GL_RGB, but 4
in the bit format.

> +     },
> +     {
> +             .format = DRM_FORMAT_ABGR2101010,
> +             .opaque_substitute = DRM_FORMAT_XBGR2101010,
> +             .gl_type = GL_RGBA,
> +             .gl_format = GL_UNSIGNED_INT_2_10_10_10_REV_EXT,

Looks flawed as earlier.

> +     },
> +     {
> +             .format = DRM_FORMAT_RGBX1010102,
> +     },
> +     {
> +             .format = DRM_FORMAT_RGBA1010102,
> +             .opaque_substitute = DRM_FORMAT_RGBX1010102,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGRX1010102,
> +     },
> +     {
> +             .format = DRM_FORMAT_BGRA1010102,
> +             .opaque_substitute = DRM_FORMAT_BGRX1010102,
> +     },
> +     {
> +             .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.

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

> +     },
> +     {
> +             .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,
> +     },
> +     {
> +             /* 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,
> +     },
> +     {
> +             .format = DRM_FORMAT_NV12,
> +             .sampler_type = EGL_TEXTURE_Y_UV_WL,
> +             .num_planes = 2,
> +             .hsub = 2,
> +             .vsub = 2,
> +     },
> +     {
> +             .format = DRM_FORMAT_NV21,
> +             .sampler_type = EGL_TEXTURE_Y_UV_WL,
> +             .num_planes = 2,
> +             .chroma_order = ORDER_VU,
> +             .hsub = 2,
> +             .vsub = 2,
> +     },
> +     {
> +             .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,
> +     },
> +     {
> +             .format = DRM_FORMAT_NV24,
> +             .sampler_type = EGL_TEXTURE_Y_UV_WL,
> +             .num_planes = 2,
> +     },
> +     {
> +             .format = DRM_FORMAT_NV42,
> +             .sampler_type = EGL_TEXTURE_Y_UV_WL,
> +             .num_planes = 2,
> +             .chroma_order = ORDER_VU,
> +     },
> +     {
> +             .format = DRM_FORMAT_YUV410,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .hsub = 4,
> +             .vsub = 4,
> +     },
> +     {
> +             .format = DRM_FORMAT_YVU410,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .chroma_order = ORDER_VU,
> +             .hsub = 4,
> +             .vsub = 4,
> +     },
> +     {
> +             .format = DRM_FORMAT_YUV411,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .hsub = 4,
> +             .vsub = 1,
> +     },
> +     {
> +             .format = DRM_FORMAT_YVU411,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .chroma_order = ORDER_VU,
> +             .hsub = 4,
> +             .vsub = 1,
> +     },
> +     {
> +             .format = DRM_FORMAT_YUV420,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .hsub = 2,
> +             .vsub = 2,
> +     },
> +     {
> +             .format = DRM_FORMAT_YVU420,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .chroma_order = ORDER_VU,
> +             .hsub = 2,
> +             .vsub = 2,
> +     },
> +     {
> +             .format = DRM_FORMAT_YUV422,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .hsub = 2,
> +             .vsub = 1,
> +     },
> +     {
> +             .format = DRM_FORMAT_YVU422,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .chroma_order = ORDER_VU,
> +             .hsub = 2,
> +             .vsub = 1,
> +     },
> +     {
> +             .format = DRM_FORMAT_YUV444,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +     },
> +     {
> +             .format = DRM_FORMAT_YVU444,
> +             .sampler_type = EGL_TEXTURE_Y_U_V_WL,
> +             .num_planes = 3,
> +             .chroma_order = ORDER_VU,
> +     },
> +};
> +
> +WL_EXPORT const struct pixel_format_info *
> +pixel_format_get_info(uint32_t format)
> +{
> +     unsigned int i;
> +
> +     for (i = 0; i < ARRAY_LENGTH(pixel_format_table); i++) {
> +             if (pixel_format_table[i].format == format)
> +                     return &pixel_format_table[i];
> +     }
> +
> +     return NULL;
> +}
> +
> +WL_EXPORT unsigned int
> +pixel_format_get_plane_count(const struct pixel_format_info *info)
> +{
> +     return info->num_planes ? info->num_planes : 1;
> +}
> +
> +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.

> +}
> +
> +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?).

> +     else
> +             return pixel_format_get_info(info->opaque_substitute);
> +}
> diff --git a/libweston/pixel-formats.h b/libweston/pixel-formats.h
> new file mode 100644
> index 0000000..8ab5035
> --- /dev/null
> +++ b/libweston/pixel-formats.h
> @@ -0,0 +1,112 @@
> +/*
> + * Copyright © 2016 Collabora, Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Author: Daniel Stone <dani...@collabora.com>
> + */
> +
> +#include <inttypes.h>
> +#include <stdbool.h>
> +
> +/**
> + * 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/

> + * sets of information. Helper functions are provided for dealing with these
> + * raw structures.
> + */
> +struct pixel_format_info {
> +     /** DRM/wl_shm format code */
> +     uint32_t format;
> +
> +     /** If non-zero, number of planes in base (non-modified) format. */
> +     int num_planes;
> +
> +     /** If format contains alpha channel, opaque equivalent of format,
> +      *  i.e. alpha channel replaced with X. */
> +     uint32_t opaque_substitute;
> +
> +     /** 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;
> +
> +     /** 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. ;-)

> +     int gl_format;
> +
> +     /** GL data type, if data can be natively/directly uploaded. */
> +     int gl_type;
> +
> +     /** If set, this format can be used with the legacy drmModeAddFB()
> +      *  function (not AddFB2), using this and the bpp member. */
> +     int depth;
> +
> +     /** See 'depth' member above. */
> +     int bpp;
> +
> +     /** Horizontal subsampling; if non-zero, divide the width by this
> +      *  member to obtain the number of columns in the source buffer for
> +      *  secondary planes only. Stride is not affected by horizontal
> +      *  subsampling. */
> +     int hsub;
> +
> +     /** Horizontal subsampling; if non-zero, divide the height by this

Should be vertical.

> +      *  member to obtain the number of rows in the source buffer for
> +      *  secondary planes only. */
> +     int vsub;
> +
> +     /* Ordering of chroma components. */
> +     enum {
> +             ORDER_UV = 0,
> +             ORDER_VU,
> +     } chroma_order;
> +
> +     /* If packed YUV (num_planes == 1), ordering of luma/chroma
> +      * components. */
> +     enum {
> +             ORDER_LUMA_CHROMA = 0,
> +             ORDER_CHROMA_LUMA,
> +     } luma_chroma_order;
> +};
> +
> +const struct pixel_format_info *pixel_format_get_info(uint32_t format);
> +
> +unsigned int
> +pixel_format_get_plane_count(const struct pixel_format_info *format);
> +
> +bool pixel_format_is_opaque(const struct pixel_format_info *format);
> +
> +const struct pixel_format_info *
> +pixel_format_get_opaque_substitute(const struct pixel_format_info *format);
> +
> +unsigned int
> +pixel_format_width_for_plane(const struct pixel_format_info *format,
> +                          unsigned int plane);
> +unsigned int
> +pixel_format_height_for_plane(const struct pixel_format_info *format,
> +                           unsigned int plane);

Very nice, very boring. Hurrah!

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

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


Thanks,
pq

Attachment: pgpp45Vn3aIAb.pgp
Description: OpenPGP digital signature

_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to