[Mesa-dev] [PATCH 03/13] gallium: Introduce 32-bit bytewise format names

[Michel Dänzer]

On Mit, 2013-05-22 at 01:56 -0700, Jose Fonseca wrote:
> ----- Original Message -----
> > On Die, 2013-05-21 at 23:15 -0700, Jose Fonseca wrote:
> > > ----- Original Message -----
> > > > Jose Fonseca <jfonseca at vmware.com> writes:
> > > > > ----- Original Message -----
> > > > >> From: Richard Sandiford <r.sandiford at uk.ibm.com>
> > > > >> 
> > > > >> RGBA8888 has R at byte 0 and A at byte 3, regardless of platform
> > > > >> endianness.
> > > > >
> > > > > Maybe I'm missing something, but this naming convention seems to me
> > > > > the exact opposite of what was decided [1], which is:
> > > > >
> > > > >  - R at byte 0, ..., and A at byte 3, regardless of platform endianness
> > > > >  would be called "R8G8B8A8"
> > > > >
> > > > >  - R at bit 0, ..., A at bit 24, encoded as integers that match the
> > > > >  platform endianness would be called "RGBA8888"
> > > > >
> > > > > which would be consistent with (as in a superset of) D3D10 format
> > > > > naming.
> > > > 
> > > > Yeah, it's supposed to be that way round in the patches.  RGBA8888 is a
> > > > 32-bit int with R in the high 8 bits and A in the low 8 bits.
> > > 
> > > This is still slightly different than what I expected: gallium was
> > > using the convention the components started with least significant bit
> > > appear first in name (same as D3D10). That is, RGBA8888 is a 32-bit
> > > int with R in the _low_ 8 bits and A in the _high_ 8 bits.
> > > 
> > > I understand that Mesa formats follow the opposite convention, but
> > > between consistency with Mesa vs consistency within gallium I believe
> > > that it is better for gallium formats to be consistent among
> > > themselves: it is much easier to remember that _all_ gallium formats
> > > go from least->most significant bit/byte/word, than to remember which
> > > formats are supposed to go from the low->high vs high->low, which
> > > would end up forcing developers to either guess wrongly or waste time
> > > look up the format implementation.
> > 
> > I disagree. There is no need or point to even think in terms of
> > least/most significant anything (implying a certain byte order) for
> > formats such as R8G8B8A8. They're just arrays.
> 
> Quite true, but this very patch (gallium: Introduce 32-bit bytewise
> format names), renames PIPE_FORMAT_B8G8R8A8_UNORM into
> PIPE_FORMAT_ARGB8888_UNORM, so I don't see how I can avoid this
> question.

Hmm, I had assumed it was done this way to avoid churn, as a lot of code
was effectively treating the existing formats like this. But thinking
about it now, I understand your concern.

> In this case I must insist to leave all array rgba8 formats alone
> (i.e., not make them endianess dependent) -- actually a good idea
> regardless (*) --, and add RGBA8888/ABGR8888 endian dependent
> aliases/alternatives instead.

Makes sense.

> Anyway, note I was not even talking of matching array formats: I'm
> talking of matching the other packed formats that this series has not
> addressed yet (e.g., PIPE_FORMAT_B5G6R5_UNORM) and some packed formats
> which may never address (e.g., PIPE_FORMAT_R11G11B10_FLOAT,
> PIPE_FORMAT_Z24_UNORM_S8_UINT), and consistency with D3D10 format
> naming (e.g., DXGI_FORMAT_B5G6R5_UNORM).

I see, thanks for the clarification.

> > For packed formats such as RGBA8888, the order used in these patches
> > (which is what I suggested in my proposal) matches the order humans use
> > for digits of numbers, as well as the Mesa formats. That seems more
> > important to me than 'matching' any non-packed formats (which only makes
> > sense if one presumes little endian byte order).
> 
> I'm sorry I didn't notice this was what you proposed earlier..
> 
> However I don't think that consistency with Mesa formats is strong:
> Mesa formats even have both ways with their *_REV variants.
> 
> So I prefer that we keep existing low->high bit/byte/word/etc naming
> convention for gallium formats.

Fair enough.


> I do appreciate all the work and thought that went on this series so far, and I really want to get this in.  So here is a summary of what's needed from my POV to get this in mergeable state:
> 
> - leave r8g8b8a8 variants alone (ie, as endianess independent)
> 
> - fix the util_format_description::is_array == TRUE && util_format_description::is_bitmask == TRUE ambiguity, either:
> 
>    - add new rgba8888/argb8888 formats for endianess dependent formats
> 
>         - add a new field on util_format_description (e.g., native_endian) for the endianess formats (**)
> 
>    - or add rgba8888/argb8888 #define
> 
>         - make sure util_format_description::is_array is set for r8g8b8a8 variants, but util_format_description::is_bitmask is not

Is there any drawback to the latter approach for formats where it's
feasible? If not, it might reduce code duplication somewhat.

> (**) Actually, I'm surprised that formats like
> PIPE_FORMAT_B5G6R5_UNORM aren't busted on big-endiang without this, as
> they haven't been converted yet, so they need to be handled precisely
> as before, right? I suppose everything was busted before, so no net
> change here

Exactly. :\

I think is_bitmask should be enough for that though, if that is
explicitly defined to imply host byte order, so no new field should be
necessary?


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer