[PATCH weston v9 01/62] libweston: Add pixel-format helpers
Pekka Paalanen
ppaalanen at gmail.com
Mon Mar 20 15:23:19 UTC 2017
On Fri, 3 Mar 2017 23:05:12 +0000
Daniel Stone <daniels at collabora.com> wrote:
> Rather than duplicating knowledge of pixel formats across several
> components, create a custom central repository.
>
> Differential Revision: https://phabricator.freedesktop.org/D1511
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
> Makefile.am | 2 +
> libweston/pixel-formats.c | 403 ++++++++++++++++++++++++++++++++++++++++++++++
> libweston/pixel-formats.h | 112 +++++++++++++
> 3 files changed, 517 insertions(+)
> create mode 100644 libweston/pixel-formats.c
> create mode 100644 libweston/pixel-formats.h
>
> v9: Go through format definitions and clean up for endianness, as well
> as remove GL_BGRA_EXT where not supported. (Pekka)
>
Hi,
some new comments, some old. I think I have checked everything
carefully, those marked as "Correct" are just to say I now agree even
if I happened to disagree earlier. I have also checked the parts I
snipped out.
> diff --git a/Makefile.am b/Makefile.am
> index 519d911..7b24d40 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -105,6 +105,8 @@ libweston_ at LIBWESTON_MAJOR@_la_SOURCES = \
> libweston/timeline-object.h \
> libweston/linux-dmabuf.c \
> libweston/linux-dmabuf.h \
> + libweston/pixel-formats.c \
> + libweston/pixel-formats.h \
> shared/helpers.h \
> shared/matrix.c \
> shared/matrix.h \
> diff --git a/libweston/pixel-formats.c b/libweston/pixel-formats.c
> new file mode 100644
> index 0000000..6dc8c9d
> --- /dev/null
> +++ b/libweston/pixel-formats.c
> @@ -0,0 +1,403 @@
> +/*
> + * 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 <daniels at collabora.com>
> + */
> +
> +#include "config.h"
> +
> +#include <endian.h>
> +#include <inttypes.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <drm/drm_fourcc.h>
Did this include line need a fix?
> +
> +#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>
Being built into libweston proper, this probably doesn't work too well
with --disable-egl... but I understand making this build without GL
headers would be painful.
Hmm, or maybe:
#if ENABLE_EGL
#define SET_GL_FORMAT(fmt) .gl_format = (fmt)
#define SET_GL_TYPE(type) .gl_type = (type)
#else
#define SET_GL_FORMAT(fmt) .gl_format = 0
#define SET_GL_TYPE(type) .gl_type = 0
#endif
perhaps?
> +
> +#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,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_format = GL_RGBA,
> + .gl_type = GL_UNSIGNED_SHORT_4_4_4_4,
> +#endif
This reminds me, documentation should probably mention this case: you
have to check the format is not opaque before you can rely on the
sampled alpha value in GL.
> + },
> + {
> + .format = DRM_FORMAT_RGBA4444,
> + .opaque_substitute = DRM_FORMAT_RGBX4444,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_format = GL_RGBA,
> + .gl_type = GL_UNSIGNED_SHORT_4_4_4_4,
> +#endif
> + },
> + {
> + .format = DRM_FORMAT_BGRX4444,
> + },
> + {
> + .format = DRM_FORMAT_BGRA4444,
> + .opaque_substitute = DRM_FORMAT_BGRX4444,
> + },
> + {
> + .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,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_format = GL_RGBA,
> + .gl_type = GL_UNSIGNED_SHORT_5_5_5_1,
> +#endif
> + },
> + {
> + .format = DRM_FORMAT_RGBA5551,
> + .opaque_substitute = DRM_FORMAT_RGBX5551,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_format = GL_RGBA,
> + .gl_type = GL_UNSIGNED_SHORT_5_5_5_1,
Correct.
> +#endif
> + },
> + {
> + .format = DRM_FORMAT_BGRX5551,
> + },
> + {
> + .format = DRM_FORMAT_BGRA5551,
> + .opaque_substitute = DRM_FORMAT_BGRX5551,
> + },
> + {
> + .format = DRM_FORMAT_RGB565,
> + .depth = 16,
> + .bpp = 16,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_format = GL_RGB,
> + .gl_type = GL_UNSIGNED_SHORT_5_6_5,
Correct.
> +#endif
> + },
> + {
> + .format = DRM_FORMAT_BGR565,
> + },
> + {
> + .format = DRM_FORMAT_RGB888,
> + },
> + {
> + .format = DRM_FORMAT_BGR888,
> + .gl_type = GL_RGB,
> + .gl_format = GL_UNSIGNED_BYTE,
> + },
> + {
> + .format = DRM_FORMAT_XRGB8888,
> + .depth = 24,
> + .bpp = 32,
> + .gl_format = GL_BGRA_EXT,
> + .gl_type = GL_UNSIGNED_BYTE,
> + },
> + {
> + .format = DRM_FORMAT_ARGB8888,
> + .opaque_substitute = DRM_FORMAT_XRGB8888,
> + .depth = 32,
> + .bpp = 32,
> + .gl_format = GL_BGRA_EXT,
> + .gl_type = GL_UNSIGNED_BYTE,
> + },
> + {
> + .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,
Correct.
> + },
> + {
> + .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,
> + },
> + {
> + .format = DRM_FORMAT_XBGR2101010,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_type = GL_RGBA,
> + .gl_format = GL_UNSIGNED_INT_2_10_10_10_REV_EXT,
> +#endif
> + },
> + {
> + .format = DRM_FORMAT_ABGR2101010,
> + .opaque_substitute = DRM_FORMAT_XBGR2101010,
> +# if __BYTE_ORDER == __LITTLE_ENDIAN
> + .gl_type = GL_RGBA,
> + .gl_format = GL_UNSIGNED_INT_2_10_10_10_REV_EXT,
Correct.
> +#endif
> + },
> + {
> + .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,
> + },
> + {
> + .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,
Missing .hsub = 2?
> + },
> + {
> + .format = DRM_FORMAT_VYUY,
> + .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
> + .num_planes = 1,
> + .luma_chroma_order = ORDER_CHROMA_LUMA,
> + .chroma_order = ORDER_VU,
Missing .hsub = 2?
> + },
> + {
> + /* 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,
To be deleted as discussed, can be added later with actual support.
> + },
> +
> +WL_EXPORT const struct pixel_format_info *
> +pixel_format_get_info(uint32_t format)
Documentation could say that this is for DRM formats specifically.
> +{
> + 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;
> +}
> +
> +WL_EXPORT const struct pixel_format_info *
> +pixel_format_get_opaque_substitute(const struct pixel_format_info *info)
> +{
> + if (!info->opaque_substitute)
> + return info;
> + else
> + return pixel_format_get_info(info->opaque_substitute);
> +}
I think you promised docs for these. ;-)
Particularly for the get_opaque_subtitute() you had a good rationale on
why it does this.
> diff --git a/libweston/pixel-formats.h b/libweston/pixel-formats.h
> new file mode 100644
> index 0000000..c3dcef0
> --- /dev/null
> +++ b/libweston/pixel-formats.h
> @@ -0,0 +1,112 @@
> +/*
> + * Copyright © 2016 Collabora, Ltd.
> + * Author: Daniel Stone <daniels at 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.
Correct up to here.
> 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. */
"read in this big-endian order" confuses me. These GL-formats are
machine-endian, aren't they?
I think it is correct that the DRM format and GL format names match in
these cases. And DRM formats are explicitly little-endian, the
description in GL spec[1] matches, so I don't understand the comment
about "big-endian order".
Now that I think of it, they seem to match only if the machine is
little-endian. If the machine is big-endian, the bit pattern in memory
described by this kind of a GL format (i.e. not GL_{UNSIGNED_}BYTE) is
different from what it was in a little-endian machine.
The corollary of that is that:
- DRM format code <-> { <whatever>, GL_UNSIGNED_BYTE } mapping is
independent of machine endianess, while
- DRM format <-> { <whatever>, GL_UNSIGNED_{INT,SHORT}_* } mapping
depends on the machine endianess.
So it's exactly the opposite of what I was thinking the last time. I
think...
Anyway, I pretty much agree with the patch.
[1] https://khronos.org/registry/OpenGL/specs/gl/glspec45.core.pdf
E.g table 8.7.
> + int gl_format;
> +
> + /** GL data type, if data can be natively/directly uploaded. */
> + int gl_type;
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170320/3bc1bcb4/attachment.sig>
More information about the wayland-devel
mailing list