On Mon, 13 Feb 2017 18:24:58 +0000 Daniel Stone <dan...@fooishbar.org> wrote:
> Hi, > > On 14 December 2016 at 15:59, Pekka Paalanen <ppaala...@gmail.com> wrote: > > On Fri, 9 Dec 2016 19:57:16 +0000 > > Daniel Stone <dani...@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
pgpQ_e6HP3Vqj.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel