Created attachment 197529 [details]
Expected result

Right, Cogl doesn't currently have a CoglPixelFormat for A2BGR10 or X2BGR10 and cogl-texture-pixmap-x11.c:_cogl_texture_pixmap_x11_update_image_texture is currently overly simplistic in that it only expects to handle three formats, RGB_888, RGBA_8888_PRE and 565 and it selects those solely on the bit depth.

In addition our _cogl_texture_driver_pixel_format_to_gl code would need updating to be able to convert from a new CoglPixelFormat to the equivalent GL texture format - assuming you have an extension for that? Without a corresponding GL texture format then we'd need to add some cogl-bitmap-fallback.c code for being able to do a format conversion.

I can try to help out if you are interested in adding support for this, though I don't have hardware I can test with myself.

Yes, I am interested in adding support for this. I have already dug a bit into the code and I would indeed appreciate some pointers.

Thanks.

Here is a set of patches that should fix pixel format detection on the fallback OpenGL rendering path and add support for 30-bit color depth. Input is welcome.

I am aware that I removed code for 16-bit color depth detection without re-adding it. This is because I don't seem to be able to start X at depth 16, so I couldn't test this code path:

Clutter-CRITICAL **: Unable to initialize Clutter: Unable to find suitable fbconfig for the GLX context: Failed to find any compatible fbconfigs

I will investigate this further.

Thanks.

Created attachment 199712 [details] [review]
Rename POPCOUNTL() to _cogl_util_popcountl() and move it to cogl-util.h

Since this function might be useful in some other parts of the code,
this commit renames it according to Cogl's naming convention and moves
it to the utilities files.

Created attachment 199713 [details] [review]
cogl-types: add 30-bit color depth pixel formats X101010 and 2101010

These new pixel formats are defined by setting the COGL_DEPTH_30_BIT
bit to 1 in CoglPixelFormat.

Since definitions of CoglPixelFormat have changed, projects linked
against Cogl (such as Clutter or Mutter) must be recompiled to use the
new headers.

Created attachment 199714 [details] [review]
Improve pixel format detection for fallback OpenGL rendering

The previous detection was based on bits per pixel only and would
consider bpp >= 24 as X888 or 8888 24-bit color depth formats.

This commit adds a utility function that returns a CoglPixelFormat
according to channel masks and color depth. This helps to add support
for more pixel formats.

Created attachment 199715 [details] [review]
Add support for X101010 and 2101010 pixel formats to fallback OpenGL rendering

Review of attachment 199713 [details] [review]:

::: cogl/cogl-types.h
@@ +173,3 @@
 #define COGL_AFIRST_BIT         (1 << 6)
+#define COGL_DEPTH_30_BIT       (1 << 7)
+#define COGL_PREMULT_BIT        (1 << 8)

no, this is not possible: Cogl is API and ABI stable unless explicitly stated, and changing the meaning and value of a definition is an API break.

why can't COGL_DEPTH_30_BIT be set to 1 << 8?

Ok, that makes sense. That was what I did first but in that case COGL_UNPREMULT_MASK must be changed to ~COGL_PREMULT_BIT so that it covers all the bit spectrum.

I will provide a fix.

Thanks.

Created attachment 199862 [details] [review]
cogl-types: add 30-bit color depth pixel formats X101010 and 2101010

These new pixel formats are defined by setting the COGL_DEPTH_30_BIT
bit to 1 in CoglPixelFormat.

https://bugzilla.gnome.org/show_bug.cgi?id=660188

Created attachment 199865 [details] [review]
cogl-types: add 30-bit color depth pixel formats X101010 and 2101010

These new pixel formats are defined by setting the COGL_DEPTH_30_BIT
bit to 1 in CoglPixelFormat.

sorry for taking quite a while to get back to you on this.

One thing I'm concerned about is that CoglPixelFormat has got into a bit of a spaghetti mess with the number of flags we now have. Having a few flags to be able to quickly answer a few common questions is quite nice (like does this have an alpha component) but I don't see much value in the _BGR flag for example and I'm not really sure we need a flag to categorize 30_BIT formats.

I think I'd like to see most new pixel formats that can't be represented uniquely with our existing flags simply be enumerated with sequential values and avoid adding more flags. We could claim the most significant byte as an enum byte like this...
                                
  enum        unused            4 bits for the bytes-per-pixel
|------| |---------------|     |--|
00000000 xxxxxxxx xxxxxxxx PFBA0000
                           ^ premult
                            ^ alpha first
                             ^ bgr order
                              ^ has alpha

In the future we can either dedicate unused bytes to more enum space or flags

New enums would be allocated (starting at 1?) for any new formats that can't uniquely be described using the existing flags.

Note: although I marked the least significant nibble as "for the bytes-per-pixel" technically that's not what they are used for exactly but it is relatively straight forward to extract a bytes-per-pixel from here and we can try to be more consistent going forward. Currently we can map the value here to bytes-per-pixel in the following way:

1 or 8   = 1 byte
2        = 3 bytes
3        = 4 bytes
4,5 or 6 = 2 bytes
7        = undefined

even though it's not a very clean mapping it's still probably good that we can extract the bytes-per-pixel from the format without needing an exhaustive switch for all the possible formats.

Actually now I come to look at this one other thing we may want to distinguish easily for all formats is which ones depend on the machine endianness and which don't. (e.g. RGBA_8888 depends on the endianness but RGB_565 doesn't). Currently we can also extract this fairly easily from the least significant nibble since the only cases that aren't endian dependent will have a value of 4, 5 or 6 in this nibble.

So in more detail the values in the lowest nibble currently mean:
1 or 8   = 1 byte
2        = 3 bytes and endianness dependent
3        = 4 bytes and endianness dependent
4,5 or 6 = 2 bytes and endianness independent
7        = undefined

That suggests to me that we could allocate three new values here:
9        = 2 bytes and endianness dependent
10       = 3 bytes and endianess independent
11       = 4 bytes and endianess independent

So (hoping you follow me so far :-)) for your new formats you could put 11 in the lowest nibble because they are 4bytes per pixel but the components aren't byte aligned so they must be independent of endianess

I think in your case because nothing is going to currently conflict if you put 11 in the lowest nibble you can even get away with not allocating any enum value out of the highest byte.

You could have:
COGL_PIXEL_FORMAT_RGB_101010 = 11,
COGL_PIXEL_FORMAT_BGR_101010 = 11 | COGL_BGR_BIT,
COGL_PIXEL_FORMAT_ARGB_2101010 = 11 | COGL_A_BIT | COGL_AFIRST_BIT,
COGL_PIXEL_FORMAT_ABGR_2101010  = 11 | COGL_A_BIT | COGL_AFIRST_BIT | COGL_BGR_BIT,

+ the _PRE formats

ok that's my current thoughts for how we can define the new pixel formats. I'll also try and chat with Neil Roberts about this to see if he has any further thoughts. Sorry to be fussy here, we haven't added new CoglPixelFormats in a while and want to make sure we have a clear approach that will continue to be able to support more formats in the future as consistently as possible.

I'll follow up with other feedback in a separate comment, possibly tomorrow now, since it's getting late here :-) thanks again for looking a this.

Rob's comments look good to me however I think we should be careful with the terminology here and say "the components aren't byte aligned" instead of "endianness independent". We might want to one day add an enum for uploading data and specify the order of the components within the word instead of specifying the order within memory. This is technically then "endianness independent" but it's not really a new pixel format so we wouldn't set the flag. Ie, it would more just be like this:

#if G_BYTE_ORDER == G_BIG_ENDIAN
#define COGL_PIXEL_FORMAT_ARGB_ENDIAN_INDEPENDENT COGL_PIXEL_FORMAT_ARGB_8888
#else
#define COGL_PIXEL_FORMAT_ARGB_ENDIAN_INDEPENDENT COGL_PIXEL_FORMAT_BGRA_8888
#endif

With the formats we currently have, this "components aren't byte aligned" value is coincidentally represented by bit 2 of the format. I wonder if we should try to preserve this in case we want to quickly check that value at some point. Unfortunately it doesn't work out that the three other bits of the lowest nibble are enough to distinguish the number of bytes. But I think it would make sense to pick values for the two new non-byte-aligned formats that also have bit 2 set anyway and make sure any new byte-aligned types don't have it set.

Comment on attachment 199712 [details] [review]
Rename POPCOUNTL() to _cogl_util_popcountl() and move it to cogl-util.h

Marking the popcount patch as obsolete because an equivalent patch has been pushed to master as 037c0aa88cb8eb

Thanks a lot for your feedback and sorry for this delayed reply.

I have re-worked my patches according to your comments. Please let me know how it looks like now.

Created attachment 201889 [details] [review]
Add 30-bit color depth pixel formats X101010 and 2101010

30-bit color depth formats are defined by using value 11 in the least
significant nibble of the pixel format enumeration. This nibble
encodes bytes-per-pixel and byte alignment.

The _cogl_get_format_bpp() function is updated accordingly to support
these new formats.

Created attachment 201890 [details] [review]
Improve pixel format detection for fallback OpenGL rendering

The previous detection was based on bits per pixel only and would
consider bpp >= 24 as X888 or 8888 24-bit color depth formats.

This commit adds a utility function that returns a CoglPixelFormat
according to channel masks and color depth. This helps to add support
for more pixel formats.

Created attachment 201891 [details] [review]
Add support for X101010 and 2101010 pixel formats to fallback OpenGL rendering

just wanted to apologise that I haven't had time to follow up on this. I'm currently a bit snowed under. I'm on holiday next week, so hopefully I'll get a chance then to take a look.

Ping. Any news about that?

ok, hey, yep we're still alive :-)

I'd really like to see this stuff integrated so in the last week I've been chipping a way at this and think I have a branch ready to land. I did have to edit some of your patches so I've updated the commit messages to reflect what I tweaked but still kept the attribution for your patches.

One part that I ended up reworking quite heavily though was the _cogl_util_pixel_format_from_masks() function so you'll see I have a separate commit for that now, with some of your patches rebased on top of that.

I've pushed my branch to wip/pixel-format-2101010 if you'd like to take a look; it's a bit much to attach the patches here I think.

The branch makes some clean up commits to remove the COGL_UNPREMULT_MASK, COGL_UNORDERED_MASK and COGL_PIXEL_FORMAT_{24,32} defines since these were undocumented and being miss used in Cogl.

On that branch you can ignore the patches before "moves and renames _cogl_get_format_bpp"

Although I now have a graphics card I can test this with, I'm not currently in a good position to do that so for now it would be good if you would be able to test the branch works for you if that's not too much trouble.

My hope is that next week Neil will be able to review the branch and we can land it.

- Regards

Ok Neil has reviewed the patches and flagged a couple of things that I fixed and so I've now landed the patches in master.

Thanks for pushing for this.