[PATCH weston 01/68] libweston: Add pixel-format helpers
Pekka Paalanen
ppaalanen at gmail.com
Tue Feb 14 10:30:25 UTC 2017
On Mon, 13 Feb 2017 18:24:58 +0000
Daniel Stone <daniel at fooishbar.org> wrote:
> 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.
Ah yes, I missed that point. Very good, although it assumes that all
non-opaque formats do have an opaque substitute, which fails on AYUV.
> >> + {
> >> + .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.
D'oh.
> >> + {
> >> + .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.
Yes, the GL side is clear. The unclear part was DRM_FORMAT_BGR888 which
is defined as little-endian, but since there is no such thing as a
24-bit word, what does "little-endian" mean in that case?
#define DRM_FORMAT_BGR888 fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R little endian */
I think that actually tries to say that there is a 24-bit word, which
is kind of like a 32-bit word for the byte order, except bits 24-31 do
not exist.
Yeah, hair-splitting, mostly on how the DRM description should be read.
> >> + {
> >> + .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.
I think you're right. DRM formats are always explicitly little-endian
regardless of the machine endianess. I was probably mislead by Pixman
formats which are machine-endian.
IOW, Pixman <-> DRM format conversion depends on machine endianess.
That makes drm_output_init_pixman() in upstream master broken for
big-endian, because the translation from GBM formats (identical to DRM
formats, except GBM_BO formats??) to Pixman formats does not take
machine endianess into account.
That actually raises a fun question about wl_shm formats. If Pixman
formats are machine-endian, and wl_shm formats are like DRM formats
little-endian, what happens with CAIRO_FORMAT_ARGB32 drawing?
https://www.cairographics.org/manual/cairo-Image-Surfaces.html#cairo-format-t
says it's machine-endian.
I.e. Cairo on big-endian may not work with wl_shm, because the format
is not the ones guaranteed to work in the protocol spec or
libwayland-server.
IOW, Pixman <-> wl_shm format conversion depends on machine endianess.
Or, are the special format codes WL_SHM_FORMAT_ARGB8888 and
WL_SHM_FORMAT_XRGB8888 machine-endian unlike everything else?
If these were originally designed to match Cairo formats, these would
need to be machine-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. :-)
>
> 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.)
Yeah, seeing there actually are people using big-endian still
( https://bugs.freedesktop.org/show_bug.cgi?id=99638 ).
I've always believed gl-renderer to be broken in that exact place you
mention, but now we have the question of what endianess was used to
define the first wl_shm formats.
> >> + {
> >> + .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.
Ok. No way to render 10bpc and put it out to KMS then? :-/
Well, in GLESv2 and possibly aside from converting in shader.
> >> + {
> >> + .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?
I suppose that was it, I can't remember. It probably wasn't obvious how
to infer all that info, or whether it just didn't belong here.
> >> +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 ...
AhhhHA! Seems a little... like burying in a mine in a field and
thinking I'm never going to go there. ;-)
> >> +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.
Perhaps, but IMO it makes little sense from API definition point of
view. That is, I think it is surprising if you didn't read the
implementation.
But again this would only be a problem for AYUV which is both not
opaque and does not have an opaque substitute.
(Missed a ! there btw.)
> >> +/**
> >> + * 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.
Hmm, I was thinking only about the literal enum values differing, but
now there is the question if the formatf also differ by endianess
definition.
> >> + /** 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?
That was probably a reference to the brainfart of thinking Pixman
formats instead of DRM formats.
> >> + /** 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.
Ok.
> > 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.
Cool, there was at least that one surprise. :-)
Thanksl,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170214/11c24e10/attachment.sig>
More information about the wayland-devel
mailing list