On Fri, 3 Mar 2017 23:05:12 +0000 Daniel Stone <dani...@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 <dani...@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_@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 <dani...@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 <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. 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
pgpR35F6LBXB8.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
