GNOME Bugzilla – Bug 671016
INVALID_ENUM errors in gnome-shell
Last modified: 2012-03-05 19:49:29 UTC
The summary has the net effect of the bug. The detailed explanation is as follows: St tries to cogl_texture_get_data from a RGBA_8888_PRE texture as A_8. Since A_8 has an alpha component, and the original data is premultiplied, cogl_texture_get_data marks the premul bit in the request format, leading to a value that _cogl_texture_driver_pixel_format_to_gl cannot handle. This in turn results in undefined (most often garbage) data fed to libGL. Patch coming.
Created attachment 208647 [details] [review] cogl-texture: don't try to premultiply A8 textures COGL_PIXEL_FORMAT_A_8 is marked as having an alpha component, but adding the premultiplied bit to it results in an invalid format (and in fact, it doesn't make sense to premultiply alpha only). Fixes a GL error INVALID_ENUM when retrieving that format from a premultiplied texture.
Thanks for the patch. I think there are a few other places that have the same problem so I would quite like to add a macro to check for this case instead. I'll attach a different patch. I don't think it would be a good idea to add a default handler to the pixel_format_to_gl function because then if we add a new format we won't get a warning reminding us to update the function.
Created attachment 208683 [details] [review] Add a conformance test for reading back an RGBA texture as alpha-only This just creates a 1x1 RGBA texture and then reads it back in COGL_PIXEL_FORMAT_A_8 format. Gnome Shell is doing this to create a shadow and I accidentally broke it so this should hopefully stop that happening again.
Created attachment 208684 [details] [review] Assert that we get a valid format in pixel_format_to_gl{,es} The assert could use a 'default:' label but that would stop GCC from giving a warning when a new enum value is added.
Created attachment 208685 [details] [review] Avoid making up the format COGL_PIXEL_FORMAT_A_8_PRE There are a few places in Cogl that try to set the premult bit on a pixel format depending on whether it has an alpha channel. However this breaks if the pixel format is alpha-only because premultiplying data without any RGB components doesn't make any sense. This adds an internal macro to check for cases where we should add the premult bit called COGL_PIXEL_FORMAT_CAN_HAVE_PREMULT. This now gets used in all places that previously just checking for COGL_A_BIT.
Review of attachment 208684 [details] [review]: looks good to me
Review of attachment 208683 [details] [review]: looks okay to me
Review of attachment 208685 [details] [review]: minor nitpick: can-have reminds me of lolcode; can-be, maybe? other than that, it looks okay to me.
I've pushed the patches as 59fd9b49d..323adc0