I've already started a lot of this work.  I've got an implementation of an array format enum and master conversion function in this branch:

http://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/format-convert-v0.1

The code there is very rough and doesn't build (because we're missing GLuint packing functions) but should give an idea of where I'm going with it.  I've also got a bunch of the auto-generation work started here:

http://cgit.freedesktop.org/~jekstrand/mesa/log/?h=format-pack-v1

I sent those to the list about a month ago and got no review.  If someone wants to pick it up from here, they're more than welcome.

Hi Jason: Samuel and I will continue your work. We will ask here if we need input from your side.

(In reply to Jason Ekstrand from comment #0)
(...)
>  5) Make a masater conversion function that takes a void*, format, width,
> height, and stride for both source and destination and just does the
> conversion.  If the above mentioned array format enum is distinct from the
> mesa_format enum, the function could be written to take a uint32_t type and
> accept either mesa_format or an array format in the same parameter.
>  6) Use the above master conversion function for TexSubimage, TexImage,
> GetTexImage, DrawPixels, and ReadPixels.  We still have to deal with pixel
> conversion, but it should vastly simplify all of them.

As far as I can see, functions in textstore do not receive a  mesa_format for their src argument, instead they receive the GL format and data type, so in order to use the master function here we need to map this combination to a mesa_format or mesa_array_format first. It does not look like this conversion is available anywhere though, so I think we need to create a function that maps (GLformat, GLDataType)-> mesa_format. Something similar to what 
_mesa_format_to_type_and_comps does, but the other way around. Jason, does this make sense to you?

(In reply to Iago Toral from comment #3)
> (...)
> As far as I can see, functions in textstore do not receive a  mesa_format
> for their src argument, instead they receive the GL format and data type, so
> in order to use the master function here we need to map this combination to
> a mesa_format or mesa_array_format first. It does not look like this
> conversion is available anywhere though, so I think we need to create a
> function that maps (GLformat, GLDataType)-> mesa_format. Something similar
> to what 
> _mesa_format_to_type_and_comps does, but the other way around. Jason, does
> this make sense to you?

Yes, that is more or less what I was intending.  It shouldn't be hard to write a function that takes a GL format and type and kicks out a mesa_format or mesa_array_format.  For most things it will be an array format.  For other things like GL_RGBA with GL_UNSIGNED_INT_8_8_8_8_REV or GL_UNSIGNED_INT_5_5_5_1_REV, you would kick out a mesa_format.  There aren't many of these, so it should be a simple switch statement.

Jason, in your initial implementation of the master function you have this assert:

if (src_array_format.as_uint && dst_array_format.as_uint) {
   assert(src_array_format.normalized == dst_array_format.normalized);
   (...)
   _mesa_swizzle_and_convert(dst, dst_gl_type, dst_array_format.num_channels,
                             src, src_gl_type, src_array_format.num_channels,
                             src2dst, normalized, width);
}

I think this assertion is wrong, since it prevents conversion from float array types (which are being generated with normalized=false) to normalized integer types. Something that is explicitly supported in _mesa_swizzle_and_convert. I am hitting this assertion while converting from RGB FLOAT to RGBA UBYTE for example.

Looking at the implementation of texstore_swizzle, which uses _mesa_swizzle_and_convert (and is the path that the original code takes to do this conversion in my example), I don't find this kind of checks. What it does is this:

is_array = _mesa_format_to_array(dstFormat, &dst_type, &dst_components,
                                 rgba2dst, &normalized);
(...)
normalized |= !_mesa_is_enum_format_integer(srcFormat);

This will make it so that normalized is always set to true for float src formats like RGB FLOAT. 

So assuming that this code is correct I think we would want to do the same in the master function: remove the assert and set normalized to true if either src or dst are normalized or if src is a float type.

There is another issue, and that is how we decide if a (glformat,gldatatype) combination is normalized or not when creating an array format from it. There are a bunch of functions in glformats.c I could use, like:

GLboolean _mesa_is_enum_format_unorm(GLenum format)
GLboolean _mesa_is_enum_format_snorm(GLenum format)
GLboolean _mesa_is_enum_format_integer(GLenum format)
...

But these do not consider the data type, only the format, so for example _mesa_is_enum_format_unorm(GL_RGB) will always return true, even if we have a GL_FLOAT data type. This is the same thing that _mesa_swizzle_and_convert does when doing:

normalized |= !_mesa_is_enum_format_integer(srcFormat);

but it is inconsistent with your definition of array formats, since all array formats generated in format_info.c have a normalized setting of 0.

I could also avoid the problem if I simply use I just use _mesa_is_enum_format_integer(format) when mapping a (glformat,gldatatype) to a mesa_array_format, like texstore_swizzle does, but as I mentioned, that will create float array formats with normalized set to true, which is inconsistent with the array_formats you are generating, so I am not sure about the best path to take here.

What do you think?

(In reply to Iago Toral from comment #5)
> Jason, in your initial implementation of the master function you have this
> assert:
> 
> if (src_array_format.as_uint && dst_array_format.as_uint) {
>    assert(src_array_format.normalized == dst_array_format.normalized);
>    (...)
>    _mesa_swizzle_and_convert(dst, dst_gl_type, dst_array_format.num_channels,
>                              src, src_gl_type, src_array_format.num_channels,
>                              src2dst, normalized, width);
> }
> 
> I think this assertion is wrong, since it prevents conversion from float
> array types (which are being generated with normalized=false) to normalized
> integer types. Something that is explicitly supported in
> _mesa_swizzle_and_convert. I am hitting this assertion while converting from
> RGB FLOAT to RGBA UBYTE for example.
> 
> Looking at the implementation of texstore_swizzle, which uses
> _mesa_swizzle_and_convert (and is the path that the original code takes to
> do this conversion in my example), I don't find this kind of checks. What it
> does is this:
> 
> is_array = _mesa_format_to_array(dstFormat, &dst_type, &dst_components,
>                                  rgba2dst, &normalized);
> (...)
> normalized |= !_mesa_is_enum_format_integer(srcFormat);
> 
> This will make it so that normalized is always set to true for float src
> formats like RGB FLOAT. 
> 
> So assuming that this code is correct I think we would want to do the same
> in the master function: remove the assert and set normalized to true if
> either src or dst are normalized or if src is a float type.

Yes, I believe that is correct.  While I implemented the master function, it didn't get much testing.  I'ts mostly just a prototype, so bugs like that aren't surprising.

> There is another issue, and that is how we decide if a (glformat,gldatatype)
> combination is normalized or not when creating an array format from it.
> There are a bunch of functions in glformats.c I could use, like:
> 
> GLboolean _mesa_is_enum_format_unorm(GLenum format)
> GLboolean _mesa_is_enum_format_snorm(GLenum format)
> GLboolean _mesa_is_enum_format_integer(GLenum format)
> ...
> 
> But these do not consider the data type, only the format, so for example
> _mesa_is_enum_format_unorm(GL_RGB) will always return true, even if we have
> a GL_FLOAT data type. This is the same thing that _mesa_swizzle_and_convert
> does when doing:
> 
> normalized |= !_mesa_is_enum_format_integer(srcFormat);

This should be correct.  The way you detect things on the GL side is usually that the GL_RGBA type formats are normalized while the GL_RGBA_INTEGER type formats are not.

The only issue here is when you should clamp floating-point.  What we probably want is a boolean "clamp" parameter that indicates that the input should be clamped to [0, 1] before converting.  Whether a float-float conversion should be clampped depends on where the conversion is being done.
 
> but it is inconsistent with your definition of array formats, since all
> array formats generated in format_info.c have a normalized setting of 0.

The generation is quite possibly wrong there.  Again, the branches I pushed were very thrown together and not a full, tested, implementation.

> I could also avoid the problem if I simply use I just use
> _mesa_is_enum_format_integer(format) when mapping a (glformat,gldatatype) to
> a mesa_array_format, like texstore_swizzle does, but as I mentioned, that
> will create float array formats with normalized set to true, which is
> inconsistent with the array_formats you are generating, so I am not sure
> about the best path to take here.

I think that's perfectly reasonable if we fix the generation format_info code.

(In reply to Jason Ekstrand from comment #6)
> (In reply to Iago Toral from comment #5)
> > Jason, in your initial implementation of the master function you have this
> > assert:
> > 
> > if (src_array_format.as_uint && dst_array_format.as_uint) {
> >    assert(src_array_format.normalized == dst_array_format.normalized);
> >    (...)
> >    _mesa_swizzle_and_convert(dst, dst_gl_type, dst_array_format.num_channels,
> >                              src, src_gl_type, src_array_format.num_channels,
> >                              src2dst, normalized, width);
> > }
> > 
> > I think this assertion is wrong, since it prevents conversion from float
> > array types (which are being generated with normalized=false) to normalized
> > integer types. Something that is explicitly supported in
> > _mesa_swizzle_and_convert. I am hitting this assertion while converting from
> > RGB FLOAT to RGBA UBYTE for example.
> > 
> > Looking at the implementation of texstore_swizzle, which uses
> > _mesa_swizzle_and_convert (and is the path that the original code takes to
> > do this conversion in my example), I don't find this kind of checks. What it
> > does is this:
> > 
> > is_array = _mesa_format_to_array(dstFormat, &dst_type, &dst_components,
> >                                  rgba2dst, &normalized);
> > (...)
> > normalized |= !_mesa_is_enum_format_integer(srcFormat);
> > 
> > This will make it so that normalized is always set to true for float src
> > formats like RGB FLOAT. 
> > 
> > So assuming that this code is correct I think we would want to do the same
> > in the master function: remove the assert and set normalized to true if
> > either src or dst are normalized or if src is a float type.
> 
> Yes, I believe that is correct.  While I implemented the master function, it
> didn't get much testing.  I'ts mostly just a prototype, so bugs like that
> aren't surprising.

Yeah, I know, but since I don't really have any previous experience with the formats code I prefer to double check my conclusions with you just in case I am missing something or messing up.

BTW, Thanks for the fast reply!

Adding myself to CC.

Jason, piglit tests hit cases where they attempt to convert GL format and data type combinations that do not match any of the existing mesa formats.

For example GL_RGB +  GL_UNSIGNED_BYTE_2_3_3_REV (BBGG GRRR). The Only mesa format of this kind is MESA_FORMAT_B2G3R3_UNORM (RRRG GGBB).

This means that we don't have pack and unpack functions for these types, which we need to use a master conversion function. I think the natural thing to do would be to add new mesa_format types for these, together with their format_pack.c and format_unpack.c functions (which should be auto-generated too). I suppose it is okay to add new mesa_format enums, right?

(In reply to Iago Toral from comment #9)
> Jason, piglit tests hit cases where they attempt to convert GL format and
> data type combinations that do not match any of the existing mesa formats.
> 
> For example GL_RGB +  GL_UNSIGNED_BYTE_2_3_3_REV (BBGG GRRR). The Only mesa
> format of this kind is MESA_FORMAT_B2G3R3_UNORM (RRRG GGBB).
> 
> This means that we don't have pack and unpack functions for these types,
> which we need to use a master conversion function. I think the natural thing
> to do would be to add new mesa_format types for these, together with their
> format_pack.c and format_unpack.c functions (which should be auto-generated
> too). I suppose it is okay to add new mesa_format enums, right?

BTW, as an added bonus, with this approach we will speed up conversion for some of these types too. For example, the way Mesa currently handles GL_UNSIGNED_BYTE_2_3_3_REV to GL_RGBA UBYTE involves two conversions (2_3_3_REV -> RGBA FLOAT -> RGBA_UBYTE), while we would be able to do that in one go via the auto-generated unpack function.

(In reply to Iago Toral from comment #10)
> (In reply to Iago Toral from comment #9)
> > Jason, piglit tests hit cases where they attempt to convert GL format and
> > data type combinations that do not match any of the existing mesa formats.
> > 
> > For example GL_RGB +  GL_UNSIGNED_BYTE_2_3_3_REV (BBGG GRRR). The Only mesa
> > format of this kind is MESA_FORMAT_B2G3R3_UNORM (RRRG GGBB).
> > 
> > This means that we don't have pack and unpack functions for these types,
> > which we need to use a master conversion function. I think the natural thing
> > to do would be to add new mesa_format types for these, together with their
> > format_pack.c and format_unpack.c functions (which should be auto-generated
> > too). I suppose it is okay to add new mesa_format enums, right?
> 
> BTW, as an added bonus, with this approach we will speed up conversion for
> some of these types too. For example, the way Mesa currently handles
> GL_UNSIGNED_BYTE_2_3_3_REV to GL_RGBA UBYTE involves two conversions
> (2_3_3_REV -> RGBA FLOAT -> RGBA_UBYTE), while we would be able to do that
> in one go via the auto-generated unpack function.

How many formats like this are there?  If it's only a few, then it probably makes sense to add the few mesa_formats that we need.

(In reply to Jason Ekstrand from comment #11)
> (In reply to Iago Toral from comment #10)
> > (In reply to Iago Toral from comment #9)
> > > Jason, piglit tests hit cases where they attempt to convert GL format and
> > > data type combinations that do not match any of the existing mesa formats.
> > > 
> > > For example GL_RGB +  GL_UNSIGNED_BYTE_2_3_3_REV (BBGG GRRR). The Only mesa
> > > format of this kind is MESA_FORMAT_B2G3R3_UNORM (RRRG GGBB).
> > > 
> > > This means that we don't have pack and unpack functions for these types,
> > > which we need to use a master conversion function. I think the natural thing
> > > to do would be to add new mesa_format types for these, together with their
> > > format_pack.c and format_unpack.c functions (which should be auto-generated
> > > too). I suppose it is okay to add new mesa_format enums, right?
> > 
> > BTW, as an added bonus, with this approach we will speed up conversion for
> > some of these types too. For example, the way Mesa currently handles
> > GL_UNSIGNED_BYTE_2_3_3_REV to GL_RGBA UBYTE involves two conversions
> > (2_3_3_REV -> RGBA FLOAT -> RGBA_UBYTE), while we would be able to do that
> > in one go via the auto-generated unpack function.
> 
> How many formats like this are there?  If it's only a few, then it probably
> makes sense to add the few mesa_formats that we need.

I don't know yet. For that I would have enable the master convertion function for all code paths, then run all the piglit tests and then check the cases that hit the assertion I have one by one removing duplicate cases, so it would take some time.

In any case, even if these were a significant bunch: do we have a good alternative? If we don't create mesa_formats for these types we would have to handle them as exceptions to the process (and this kind of defeats the purpose of a master function). We would have to handle conversions from and to these types through different paths and write the conversions functions we need by hand...

(In reply to Iago Toral from comment #12)
> (In reply to Jason Ekstrand from comment #11)
> > (In reply to Iago Toral from comment #10)
> > > (In reply to Iago Toral from comment #9)
> > > > Jason, piglit tests hit cases where they attempt to convert GL format and
> > > > data type combinations that do not match any of the existing mesa formats.
> > > > 
> > > > For example GL_RGB +  GL_UNSIGNED_BYTE_2_3_3_REV (BBGG GRRR). The Only mesa
> > > > format of this kind is MESA_FORMAT_B2G3R3_UNORM (RRRG GGBB).
> > > > 
> > > > This means that we don't have pack and unpack functions for these types,
> > > > which we need to use a master conversion function. I think the natural thing
> > > > to do would be to add new mesa_format types for these, together with their
> > > > format_pack.c and format_unpack.c functions (which should be auto-generated
> > > > too). I suppose it is okay to add new mesa_format enums, right?
> > > 
> > > BTW, as an added bonus, with this approach we will speed up conversion for
> > > some of these types too. For example, the way Mesa currently handles
> > > GL_UNSIGNED_BYTE_2_3_3_REV to GL_RGBA UBYTE involves two conversions
> > > (2_3_3_REV -> RGBA FLOAT -> RGBA_UBYTE), while we would be able to do that
> > > in one go via the auto-generated unpack function.
> > 
> > How many formats like this are there?  If it's only a few, then it probably
> > makes sense to add the few mesa_formats that we need.
> 
> I don't know yet. For that I would have enable the master convertion
> function for all code paths, then run all the piglit tests and then check
> the cases that hit the assertion I have one by one removing duplicate cases,
> so it would take some time.
> 
> In any case, even if these were a significant bunch: do we have a good
> alternative? If we don't create mesa_formats for these types we would have
> to handle them as exceptions to the process (and this kind of defeats the
> purpose of a master function). We would have to handle conversions from and
> to these types through different paths and write the conversions functions
> we need by hand...

You should know once you write a gl_format_and_type_to_mesa_format function.  I don't think there will be many.  I think OpenGL only specifies about 8 packed formats (plus swizzling) and we should already have most of them.

(In reply to Jason Ekstrand from comment #13)
> (In reply to Iago Toral from comment #12)
> > (In reply to Jason Ekstrand from comment #11)
> > > (In reply to Iago Toral from comment #10)
> > > > (In reply to Iago Toral from comment #9)
> > > > > Jason, piglit tests hit cases where they attempt to convert GL format and
> > > > > data type combinations that do not match any of the existing mesa formats.
> > > > > 
> > > > > For example GL_RGB +  GL_UNSIGNED_BYTE_2_3_3_REV (BBGG GRRR). The Only mesa
> > > > > format of this kind is MESA_FORMAT_B2G3R3_UNORM (RRRG GGBB).
> > > > > 
> > > > > This means that we don't have pack and unpack functions for these types,
> > > > > which we need to use a master conversion function. I think the natural thing
> > > > > to do would be to add new mesa_format types for these, together with their
> > > > > format_pack.c and format_unpack.c functions (which should be auto-generated
> > > > > too). I suppose it is okay to add new mesa_format enums, right?
> > > > 
> > > > BTW, as an added bonus, with this approach we will speed up conversion for
> > > > some of these types too. For example, the way Mesa currently handles
> > > > GL_UNSIGNED_BYTE_2_3_3_REV to GL_RGBA UBYTE involves two conversions
> > > > (2_3_3_REV -> RGBA FLOAT -> RGBA_UBYTE), while we would be able to do that
> > > > in one go via the auto-generated unpack function.
> > > 
> > > How many formats like this are there?  If it's only a few, then it probably
> > > makes sense to add the few mesa_formats that we need.
> > 
> > I don't know yet. For that I would have enable the master convertion
> > function for all code paths, then run all the piglit tests and then check
> > the cases that hit the assertion I have one by one removing duplicate cases,
> > so it would take some time.
> > 
> > In any case, even if these were a significant bunch: do we have a good
> > alternative? If we don't create mesa_formats for these types we would have
> > to handle them as exceptions to the process (and this kind of defeats the
> > purpose of a master function). We would have to handle conversions from and
> > to these types through different paths and write the conversions functions
> > we need by hand...
> 
> You should know once you write a gl_format_and_type_to_mesa_format function.

I have that already, but it is not enough. At the moment, if I have detected that a format is not an array format, I do something like this to decide if it has a matching mesa format:

   for (int f = 1; f < MESA_FORMAT_COUNT; f++)
      if (_mesa_format_matches_format_and_type(f, format, type, swap_bytes))
         return f;

So the cases that don't match simply continue and hit an assertion.

> I don't think there will be many.  I think OpenGL only specifies about 8
> packed formats (plus swizzling) and we should already have most of them.

Let's assume they are not that many then.

On a different note, I have just noticed that the driver can select a different texture format than the internal format specified by the client (glTexImage*), when the specified format is not supported. This creates a requirement for swizzle transformations where we need to do src->baseinternal->rgba->dst, but the master function, as it is right now, does not know about the internal format (only knows src and dst, so it does src->rgba->dst), so it fails for some of these cases.

For example, in one case I see that the client specifies MESA_FORMAT_I_SINT8 (swizzle 0000) as the internal format for the texture, but the driver does not support that and uses MESA_FORMAT_RGBA_SINT8 (swizzle 0123) instead. A master function that only knows about MESA_FORMAT_RGBA_SINT8 and does not know that the format requested by the client was MESA_FORMAT_I_SINT8 will not produce correct results since it would not be able to compute the right swizzle transform for _mesa_swizzle_and_convert.

So my proposal is to pass the baseinternalformat to the master converter. If there are cases where we do not care about an internalformat we can just pass _mesa_get_format_base_format(dstFormat) and then have the master converter compute a different swizzle when the provided internal format is different from _mesa_get_format_base_format(dstFormat), which is what various parts of texstore are doing now.

Sounds reasonable?

(In reply to Iago Toral from comment #14)
> (...)
> > > I don't know yet. For that I would have enable the master convertion
> > > function for all code paths, then run all the piglit tests and then check
> > > the cases that hit the assertion I have one by one removing duplicate cases,
> > > so it would take some time.
> > > 
> > > In any case, even if these were a significant bunch: do we have a good
> > > alternative? If we don't create mesa_formats for these types we would have
> > > to handle them as exceptions to the process (and this kind of defeats the
> > > purpose of a master function). We would have to handle conversions from and
> > > to these types through different paths and write the conversions functions
> > > we need by hand...
> > 
> > You should know once you write a gl_format_and_type_to_mesa_format function.
> 
> I have that already, but it is not enough. At the moment, if I have detected
> that a format is not an array format, I do something like this to decide if
> it has a matching mesa format:
> 
>    for (int f = 1; f < MESA_FORMAT_COUNT; f++)
>       if (_mesa_format_matches_format_and_type(f, format, type, swap_bytes))
>          return f;
> 
> So the cases that don't match simply continue and hit an assertion.

You should be able to do this with a simple switch statement.  There aren't that many of them.  According to the GL 1.2 docs for TexImage, there are:

GL_UNSIGNED_BYTE_3_3_2
GL_UNSIGNED_BYTE_2_3_3_REV
GL_UNSIGNED_SHORT_5_6_5
GL_UNSIGNED_SHORT_5_6_5_REV
GL_UNSIGNED_SHORT_4_4_4_4
GL_UNSIGNED_SHORT_4_4_4_4_REV
GL_UNSIGNED_SHORT_5_5_5_1
GL_UNSIGNED_SHORT_1_5_5_5_REV
GL_UNSIGNED_INT_8_8_8_8
GL_UNSIGNED_INT_8_8_8_8_REV
GL_UNSIGNED_INT_10_10_10_2
GL_UNSIGNED_INT_2_10_10_10_REV

I think they added 1 or 2 more in extensions, but that should be it.  Also, you have to watch out for GL_RGB vs. GL_BGR stuff
 
> > I don't think there will be many.  I think OpenGL only specifies about 8
> > packed formats (plus swizzling) and we should already have most of them.
> 
> Let's assume they are not that many then.
> 
> On a different note, I have just noticed that the driver can select a
> different texture format than the internal format specified by the client
> (glTexImage*), when the specified format is not supported. This creates a
> requirement for swizzle transformations where we need to do
> src->baseinternal->rgba->dst, but the master function, as it is right now,
> does not know about the internal format (only knows src and dst, so it does
> src->rgba->dst), so it fails for some of these cases.
> 
> For example, in one case I see that the client specifies MESA_FORMAT_I_SINT8
> (swizzle 0000) as the internal format for the texture, but the driver does
> not support that and uses MESA_FORMAT_RGBA_SINT8 (swizzle 0123) instead. A
> master function that only knows about MESA_FORMAT_RGBA_SINT8 and does not
> know that the format requested by the client was MESA_FORMAT_I_SINT8 will
> not produce correct results since it would not be able to compute the right
> swizzle transform for _mesa_swizzle_and_convert.
> 
> So my proposal is to pass the baseinternalformat to the master converter. If
> there are cases where we do not care about an internalformat we can just
> pass _mesa_get_format_base_format(dstFormat) and then have the master
> converter compute a different swizzle when the provided internal format is
> different from _mesa_get_format_base_format(dstFormat), which is what
> various parts of texstore are doing now.
> 
> Sounds reasonable?

Yes, we do need something for that.  Using the GL_RGB/RGBA enum would work fine.  Another option would be to have an array of 4 bools that gives a channel mask.

We also need new mesa_format enums and pack/unpack functions for byte swapped variants of non-array types.

(In reply to Iago Toral from comment #16)
> We also need new mesa_format enums and pack/unpack functions for byte
> swapped variants of non-array types.

Why?  Is this for the byte_swapped flag on GL upload/downloads?  If that's the only reason, then I'd rather not bother adding mesa types.  We can easily do a byte swap with _mesa_swizzle_and_convert.  While it's a little bit of a performance hit, people shouldn't be using that attribute anyway.

(In reply to Jason Ekstrand from comment #17)
> (In reply to Iago Toral from comment #16)
> > We also need new mesa_format enums and pack/unpack functions for byte
> > swapped variants of non-array types.
> 
> Why?  Is this for the byte_swapped flag on GL upload/downloads?  If that's
> the only reason, then I'd rather not bother adding mesa types.  We can
> easily do a byte swap with _mesa_swizzle_and_convert.  While it's a little
> bit of a performance hit, people shouldn't be using that attribute anyway.

I think that won't help in this case. We can only use _mesa_swizzle_and_convert for array types, but as I say, this involves non-array types like GL_UNSIGNED_INT_10_10_10_2. The master converter will not use _mesa_swizzle_and_convert with these, it goes through things like _mesa_unpack_rgba_row and _mesa_pack_float_rgba_row for example (which rely on auto-generated pack/unpack functions). Do you think we should try expand these to consider byte-swapping?

Jason, for conversions where we cannot use a fast path in the master converter (that is, where we need to unpack the src to rgba and then pack from rgba to dst) you differentiate between 3 paths: uint (integer), float (is_signed || bits > 8) and ubyte (otherwise).

While the last two paths seem to be always valid (I think we have pack and unpack functions for all relevant formats for these types), I am hitting an assertion in the first path because there are no uint unpack functions for some types. Indeed, _mesa_unpack_uint_rgba_row only supports unpacking from a limited set of uint formats. I am hitting this path in cases where dst is an integer type but src is not. The code will choose the integer even though the implementation requires that both src and dst are integer, since we need an uint unpack function for the src in this case.

A solution for this would be to forget about the uint path and always go with the float path or the ubyte path, but I imagine that the uint path has benefits when it can be used... I guess because we could avoid uint->{float,byte} transformations.

So, I think we have two options:
1) Auto-generate uint unpack/pack functions for all required mesa formats.
2) Only use the integer path if both src and dst are integer types.

I think 1) is only useful when src is not an integer type (so we would be doing a type conversion anyway), so maybe 2) is the most sensible approach in this case. What do you think?

(In reply to Iago Toral from comment #18)
> (In reply to Jason Ekstrand from comment #17)
> > (In reply to Iago Toral from comment #16)
> > > We also need new mesa_format enums and pack/unpack functions for byte
> > > swapped variants of non-array types.
> > 
> > Why?  Is this for the byte_swapped flag on GL upload/downloads?  If that's
> > the only reason, then I'd rather not bother adding mesa types.  We can
> > easily do a byte swap with _mesa_swizzle_and_convert.  While it's a little
> > bit of a performance hit, people shouldn't be using that attribute anyway.
> 
> I think that won't help in this case. We can only use
> _mesa_swizzle_and_convert for array types, but as I say, this involves
> non-array types like GL_UNSIGNED_INT_10_10_10_2. The master converter will
> not use _mesa_swizzle_and_convert with these, it goes through things like
> _mesa_unpack_rgba_row and _mesa_pack_float_rgba_row for example (which rely
> on auto-generated pack/unpack functions). Do you think we should try expand
> these to consider byte-swapping?

My inclination on byte-swapping it to keep it out of the master conversion function.  It's really an oddity of the old GL api and not a core format conversion thing.  No sanely written app should use byte swapping, so I'm not too worried about making it fast.  My inclination there is to keep going byteswapping and the other format conversion ops in TexImage, GetTexImag, ReadPixels, and DrawPixels.  They can call _mesa_swizzle_and_convert to do the byteswap operation befor calling into the actual format conversion function.

The downside here is that there are a few cases where we can do better.  However, that's pretty much limited to GL_UNSIGNED_INT_8_8_8_8[_REV] and I don't think it's worth the effort.

(In reply to Iago Toral from comment #19)
> Jason, for conversions where we cannot use a fast path in the master
> converter (that is, where we need to unpack the src to rgba and then pack
> from rgba to dst) you differentiate between 3 paths: uint (integer), float
> (is_signed || bits > 8) and ubyte (otherwise).
> 
> While the last two paths seem to be always valid (I think we have pack and
> unpack functions for all relevant formats for these types), I am hitting an
> assertion in the first path because there are no uint unpack functions for
> some types. Indeed, _mesa_unpack_uint_rgba_row only supports unpacking from
> a limited set of uint formats. I am hitting this path in cases where dst is
> an integer type but src is not. The code will choose the integer even though
> the implementation requires that both src and dst are integer, since we need
> an uint unpack function for the src in this case.
> 
> A solution for this would be to forget about the uint path and always go
> with the float path or the ubyte path, but I imagine that the uint path has
> benefits when it can be used... I guess because we could avoid
> uint->{float,byte} transformations.
> 
> So, I think we have two options:
> 1) Auto-generate uint unpack/pack functions for all required mesa formats.
> 2) Only use the integer path if both src and dst are integer types.
> 
> I think 1) is only useful when src is not an integer type (so we would be
> doing a type conversion anyway), so maybe 2) is the most sensible approach
> in this case. What do you think?

2 sounds like the better plan.  Can you give an example of when we are hitting that assertion?  I thought the GL spec forbid you from doing normalized -> integer conversions so I'm not seeing how this happens.

(In reply to Jason Ekstrand from comment #20)
> (In reply to Iago Toral from comment #18)
> > (In reply to Jason Ekstrand from comment #17)
> > > (In reply to Iago Toral from comment #16)
> > > > We also need new mesa_format enums and pack/unpack functions for byte
> > > > swapped variants of non-array types.
> > > 
> > > Why?  Is this for the byte_swapped flag on GL upload/downloads?  If that's
> > > the only reason, then I'd rather not bother adding mesa types.  We can
> > > easily do a byte swap with _mesa_swizzle_and_convert.  While it's a little
> > > bit of a performance hit, people shouldn't be using that attribute anyway.
> > 
> > I think that won't help in this case. We can only use
> > _mesa_swizzle_and_convert for array types, but as I say, this involves
> > non-array types like GL_UNSIGNED_INT_10_10_10_2. The master converter will
> > not use _mesa_swizzle_and_convert with these, it goes through things like
> > _mesa_unpack_rgba_row and _mesa_pack_float_rgba_row for example (which rely
> > on auto-generated pack/unpack functions). Do you think we should try expand
> > these to consider byte-swapping?
> 
> My inclination on byte-swapping it to keep it out of the master conversion
> function.  It's really an oddity of the old GL api and not a core format
> conversion thing.  No sanely written app should use byte swapping, so I'm
> not too worried about making it fast.  My inclination there is to keep going
> byteswapping and the other format conversion ops in TexImage, GetTexImag,
> ReadPixels, and DrawPixels.  They can call _mesa_swizzle_and_convert to do
> the byteswap operation befor calling into the actual format conversion
> function.

I'll try it this way then.

> The downside here is that there are a few cases where we can do better. 
> However, that's pretty much limited to GL_UNSIGNED_INT_8_8_8_8[_REV] and I
> don't think it's worth the effort.

I am already handling GL_UNSIGNED_INT_8_8_8_8[_REV] with byte swapping when we generate the mesa_array_format from the gl format and type.(it is only a matter of reverting the component mapping in this case), so if we want we can have TexImage and cia handle byte-swapping for all formats but this one.

(In reply to Jason Ekstrand from comment #21)
> (In reply to Iago Toral from comment #19)
> > Jason, for conversions where we cannot use a fast path in the master
> > converter (that is, where we need to unpack the src to rgba and then pack
> > from rgba to dst) you differentiate between 3 paths: uint (integer), float
> > (is_signed || bits > 8) and ubyte (otherwise).
> > 
> > While the last two paths seem to be always valid (I think we have pack and
> > unpack functions for all relevant formats for these types), I am hitting an
> > assertion in the first path because there are no uint unpack functions for
> > some types. Indeed, _mesa_unpack_uint_rgba_row only supports unpacking from
> > a limited set of uint formats. I am hitting this path in cases where dst is
> > an integer type but src is not. The code will choose the integer even though
> > the implementation requires that both src and dst are integer, since we need
> > an uint unpack function for the src in this case.
> > 
> > A solution for this would be to forget about the uint path and always go
> > with the float path or the ubyte path, but I imagine that the uint path has
> > benefits when it can be used... I guess because we could avoid
> > uint->{float,byte} transformations.
> > 
> > So, I think we have two options:
> > 1) Auto-generate uint unpack/pack functions for all required mesa formats.
> > 2) Only use the integer path if both src and dst are integer types.
> > 
> > I think 1) is only useful when src is not an integer type (so we would be
> > doing a type conversion anyway), so maybe 2) is the most sensible approach
> > in this case. What do you think?
> 
> 2 sounds like the better plan.  Can you give an example of when we are
> hitting that assertion?  I thought the GL spec forbid you from doing
> normalized -> integer conversions so I'm not seeing how this happens.

Sure, at least this conversion is hitting that:

src: MESA_FORMAT_B10G10R10A2_UNORM (GL_UNSIGNED_NORMALIZED)
dst: MESA_FORMAT_R10G10B10A2_UINT (GL_UNSIGNED_INT)

This comes from this piglit test:
bin/ext_texture_integer-texture_integer_glsl130

I think what is going on is that the src argument here is defined as GL_RGBA_INTEGER and GL_UNSIGNED_INT_10_10_10_2, which I map to MESA_FORMAT_A2B10G10R10_UNORM instead of MESA_FORMAT_A2B10G10R10_UINT (we don't have MESA_FORMAT_A2B10G10R10_UINT, so that's why). This works because they have the same pixel layout, so the conversion is the same, but then we hit this situation.

Should we add the mesa_formats for MESA_FORMAT_A2B10G10R10_UINT and MESA_FORMAT_A2R10G10B10_UINT too? (currently we only have MESA_FORMAT_B10G10R10A2_UINT and MESA_FORMAT_R10G10B10A2_UINT).

Jason, what do you think we should do for GL_COLOR_INDEX + GL_BITMAP?

Currently this is handled as a special case in the texstore.c ubyte path (store_ubyte_texture), specifically, the special case handles extracting indexed color data so that the incoming pixels can be represented as an RGBA image, then do the conversion from RGBA to dst normally.

We do not have a Mesa format for this combination.

I think we have two options:
1) I guess we could create a mesa_format, detect this case specifically in the master function and handle it there.
2) The other option would be to let clients deal with it. That would involve letting them transform it to RGBA before they call the master function.

What do you think?

I thought we killed color indexing with fire a while back.  Did we miss some?

(In reply to Iago Toral from comment #24)
> Jason, what do you think we should do for GL_COLOR_INDEX + GL_BITMAP?
> 
> Currently this is handled as a special case in the texstore.c ubyte path
> (store_ubyte_texture), specifically, the special case handles extracting
> indexed color data so that the incoming pixels can be represented as an RGBA
> image, then do the conversion from RGBA to dst normally.
> 
> We do not have a Mesa format for this combination.
> 
> I think we have two options:
> 1) I guess we could create a mesa_format, detect this case specifically in
> the master function and handle it there.
> 2) The other option would be to let clients deal with it. That would involve
> letting them transform it to RGBA before they call the master function.
> 
> What do you think?

Option 2.  Mesa internally hasn't supported color index in a long time, let's not add it back.

(In reply to Iago Toral from comment #23)
> (In reply to Jason Ekstrand from comment #21)
> > (In reply to Iago Toral from comment #19)
> > > Jason, for conversions where we cannot use a fast path in the master
> > > converter (that is, where we need to unpack the src to rgba and then pack
> > > from rgba to dst) you differentiate between 3 paths: uint (integer), float
> > > (is_signed || bits > 8) and ubyte (otherwise).
> > > 
> > > While the last two paths seem to be always valid (I think we have pack and
> > > unpack functions for all relevant formats for these types), I am hitting an
> > > assertion in the first path because there are no uint unpack functions for
> > > some types. Indeed, _mesa_unpack_uint_rgba_row only supports unpacking from
> > > a limited set of uint formats. I am hitting this path in cases where dst is
> > > an integer type but src is not. The code will choose the integer even though
> > > the implementation requires that both src and dst are integer, since we need
> > > an uint unpack function for the src in this case.
> > > 
> > > A solution for this would be to forget about the uint path and always go
> > > with the float path or the ubyte path, but I imagine that the uint path has
> > > benefits when it can be used... I guess because we could avoid
> > > uint->{float,byte} transformations.
> > > 
> > > So, I think we have two options:
> > > 1) Auto-generate uint unpack/pack functions for all required mesa formats.
> > > 2) Only use the integer path if both src and dst are integer types.
> > > 
> > > I think 1) is only useful when src is not an integer type (so we would be
> > > doing a type conversion anyway), so maybe 2) is the most sensible approach
> > > in this case. What do you think?
> > 
> > 2 sounds like the better plan.  Can you give an example of when we are
> > hitting that assertion?  I thought the GL spec forbid you from doing
> > normalized -> integer conversions so I'm not seeing how this happens.
> 
> Sure, at least this conversion is hitting that:
> 
> src: MESA_FORMAT_B10G10R10A2_UNORM (GL_UNSIGNED_NORMALIZED)
> dst: MESA_FORMAT_R10G10B10A2_UINT (GL_UNSIGNED_INT)
> 
> This comes from this piglit test:
> bin/ext_texture_integer-texture_integer_glsl130
> 
> I think what is going on is that the src argument here is defined as
> GL_RGBA_INTEGER and GL_UNSIGNED_INT_10_10_10_2, which I map to
> MESA_FORMAT_A2B10G10R10_UNORM instead of MESA_FORMAT_A2B10G10R10_UINT (we
> don't have MESA_FORMAT_A2B10G10R10_UINT, so that's why). This works because
> they have the same pixel layout, so the conversion is the same, but then we
> hit this situation.
> 
> Should we add the mesa_formats for MESA_FORMAT_A2B10G10R10_UINT and
> MESA_FORMAT_A2R10G10B10_UINT too? (currently we only have
> MESA_FORMAT_B10G10R10A2_UINT and MESA_FORMAT_R10G10B10A2_UINT).

Yeah, I think adding UINT versions of those is reasonable.  That's bee bothering me for a while.

(In reply to Kenneth Graunke from comment #25)
> I thought we killed color indexing with fire a while back.  Did we miss some?

I see multiple references to GL_COLOR_INDEX in src/mesa/main and at least these piglit tests using it explicitly: tests/bugs/fdo10370.c, tests/glean/treadpix.cpp.

(In reply to Jason Ekstrand from comment #26)
> (In reply to Iago Toral from comment #24)
> > Jason, what do you think we should do for GL_COLOR_INDEX + GL_BITMAP?
> > 
> > Currently this is handled as a special case in the texstore.c ubyte path
> > (store_ubyte_texture), specifically, the special case handles extracting
> > indexed color data so that the incoming pixels can be represented as an RGBA
> > image, then do the conversion from RGBA to dst normally.
> > 
> > We do not have a Mesa format for this combination.
> > 
> > I think we have two options:
> > 1) I guess we could create a mesa_format, detect this case specifically in
> > the master function and handle it there.
> > 2) The other option would be to let clients deal with it. That would involve
> > letting them transform it to RGBA before they call the master function.
> > 
> > What do you think?
> 
> Option 2.  Mesa internally hasn't supported color index in a long time,
> let's not add it back.

What do you mean by not supporting it internally? As I mention in my reply to Kenneth I see multiple references to GL_COLOR_INDEX in Mesa, there is a specific path to deal with it in texstore and there are even piglit tests for this that pass in master...

Just to be clear, when I said "clients" in option 2) I meant clients of the master function inside Mesa (so things like texstore.c). My idea was that things like texstore would handle GL_COLOR_INDEX and take care of converting it to RGBA on their own before calling the master function, but if you say that GL_COLOR_INDEX is not really supported any more, maybe we should just ignore it completely and do nothing about it in texstore and friends?

(In reply to Iago Toral from comment #29)
> (In reply to Jason Ekstrand from comment #26)
> > (In reply to Iago Toral from comment #24)
> > > Jason, what do you think we should do for GL_COLOR_INDEX + GL_BITMAP?
> > > 
> > > Currently this is handled as a special case in the texstore.c ubyte path
> > > (store_ubyte_texture), specifically, the special case handles extracting
> > > indexed color data so that the incoming pixels can be represented as an RGBA
> > > image, then do the conversion from RGBA to dst normally.
> > > 
> > > We do not have a Mesa format for this combination.
> > > 
> > > I think we have two options:
> > > 1) I guess we could create a mesa_format, detect this case specifically in
> > > the master function and handle it there.
> > > 2) The other option would be to let clients deal with it. That would involve
> > > letting them transform it to RGBA before they call the master function.
> > > 
> > > What do you think?
> > 
> > Option 2.  Mesa internally hasn't supported color index in a long time,
> > let's not add it back.
> 
> What do you mean by not supporting it internally? As I mention in my reply
> to Kenneth I see multiple references to GL_COLOR_INDEX in Mesa, there is a
> specific path to deal with it in texstore and there are even piglit tests
> for this that pass in master...

Yes, we still support uploading textures as GL_COLOR_INDEX because we have to.  However, we have not supported using indexed formats for render targets or actual internal texture formats for some time.  In fact, I recently removed the metadata about indexed formats from format_info.c.

> Just to be clear, when I said "clients" in option 2) I meant clients of the
> master function inside Mesa (so things like texstore.c). My idea was that
> things like texstore would handle GL_COLOR_INDEX and take care of converting
> it to RGBA on their own before calling the master function, but if you say
> that GL_COLOR_INDEX is not really supported any more, maybe we should just
> ignore it completely and do nothing about it in texstore and friends?

Yes, I got that.  As I said above, we still need to handle it in texture upload because it's forever part of the GL spec.  However, we would rather keep it out of core as much as possible.

Jason, there is an had-hoc path to deal with MESA_FORMAT_YCBCR* in texstore.c. Seems that this path only allows conversion between these formats, which is mostly a memcpy with some byte swapping when necessary. Should we also keep this one as a special case to be handled outside the master function?

(In reply to Iago Toral from comment #31)
> Jason, there is an had-hoc path to deal with MESA_FORMAT_YCBCR* in
> texstore.c. Seems that this path only allows conversion between these
> formats, which is mostly a memcpy with some byte swapping when necessary.
> Should we also keep this one as a special case to be handled outside the
> master function?

Yes, there's code for that in texstore.c and in format_unpack.c.  We should make that consisten either by movint the code from texstore.c to format_pack.c or move it from format_unpack.c to readpixels.  I'm ok with moving everything into the packing/unpacking functions and letting the conversion function handle it.  It is a MESA_FORMAT after all.

Jason, I would like to know your opinion about the integer RGBA clamping done in pack.c (_mesa_pack_rgba_span_from_ints()).

glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3. core specification) that for an integer RGBA color, each component is clamped to the representable range of type.

Those GL functions end up calling pack.c's functions for performing the conversion (mesa_pack_rgba_span_from_ints() and others).

It's possible to replace some of those pack.c's conversion functions by the master conversion but the problem is in the clamping operation. I would like to add a boolean argument called "clamp" to the master conversion function which is passed to _mesa_swizzle_and_convert() where each of its convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp" is true.

This "clamp" parameter would be false for every call to either master conversion function or _mesa_swizzle_and_convert() except when they are being called from pack.c.

What do you think?

(In reply to Samuel Iglesias from comment #33)
> Jason, I would like to know your opinion about the integer RGBA clamping
> done in pack.c (_mesa_pack_rgba_span_from_ints()).
> 
> glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3.
> core specification) that for an integer RGBA color, each component is
> clamped to the representable range of type.
> 
> Those GL functions end up calling pack.c's functions for performing the
> conversion (mesa_pack_rgba_span_from_ints() and others).
> 
> It's possible to replace some of those pack.c's conversion functions by the
> master conversion but the problem is in the clamping operation. I would like
> to add a boolean argument called "clamp" to the master conversion function
> which is passed to _mesa_swizzle_and_convert() where each of its
> convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp"
> is true.
> 
> This "clamp" parameter would be false for every call to either master
> conversion function or _mesa_swizzle_and_convert() except when they are
> being called from pack.c.
> 
> What do you think?

Supporting clamping is probably fine.  I think we determined we needed a clamp parameter anyway for some of the float conversions.  I guess it makes sense for integers too.  Let's watch out for performance when we implement it though.  Changing the loop inside mesa_swizzle_and_convert without hurting performance can be tricky.  The teximage-colors test in Piglit has a -benchmark flag that can be used for testing that.

Jason, you wrote some fast paths in _mesa_format_convert for the cases where we can directly pack or unpack to the destination format. In these paths, if we have to pack from or unpack to RGBA8888 UINT, you have asserts to make sure that we are packing to or unpacking from an integer type. Likewise, if we are packing from or unpacking to RGBA8888 UBYTE, you added asserts to make sure that we are packing to or unpacking from a non-integer type.

I am not sure why these were added, but I think we should remove them since there are valid conversions that will hit these paths. For example, glReadPixels where we want to store the pixel data in GL_RGBA/GL_UNSIGNED_INT. In this case, the destination format matches one of these fast path (RGBA UINT) but the source format, which is the render buffer's format, is MESA_FORMAT_B8G8R8A8_UNORM which is not integer.

I think we have at least two options:

1) Transform the asserts into conditionals. With these we make sure that these fast paths are only used when the assertions you have are not violated, but we still let _mesa_format_convert use the non-fast paths to resolve the conversion in the cases that would hit the assertions. If there was a good reason for these assertions to exist then I guess this is the way to go.

2) We simply remove the asserts. I think pack/unpack functions can deal with any format type, right? At the moment uint pack/unpack functions seem to be an exception since they only support packing to and unpacking from integer types, but since these functions are auto-generated it seems we could just let them handle other types too. Since in this case we would enable the fast path, it would be the most efficient solution.

What do you think?

(In reply to Jason Ekstrand from comment #32)
> (In reply to Iago Toral from comment #31)
> > Jason, there is an had-hoc path to deal with MESA_FORMAT_YCBCR* in
> > texstore.c. Seems that this path only allows conversion between these
> > formats, which is mostly a memcpy with some byte swapping when necessary.
> > Should we also keep this one as a special case to be handled outside the
> > master function?
> 
> Yes, there's code for that in texstore.c and in format_unpack.c.  We should
> make that consisten either by movint the code from texstore.c to
> format_pack.c or move it from format_unpack.c to readpixels.  I'm ok with
> moving everything into the packing/unpacking functions and letting the
> conversion function handle it.  It is a MESA_FORMAT after all.

Looking at _mesa_texstore_ycbcr() inside texstore.c, it is just a memcpy without any RGB to YCBCR conversion.

Do we need to implement RGB to YCBCR conversion in format_pack? Or we just need to use a special case for that format in _mesa_format_convert (so that it uses the function that memcopies instead of pack/unpack)? The second option would be more efficient.

(In reply to Iago Toral from comment #35)
> Jason, you wrote some fast paths in _mesa_format_convert for the cases where
> we can directly pack or unpack to the destination format. In these paths, if
> we have to pack from or unpack to RGBA8888 UINT, you have asserts to make
> sure that we are packing to or unpacking from an integer type. Likewise, if
> we are packing from or unpacking to RGBA8888 UBYTE, you added asserts to
> make sure that we are packing to or unpacking from a non-integer type.
> 
> I am not sure why these were added, but I think we should remove them since
> there are valid conversions that will hit these paths. For example,
> glReadPixels where we want to store the pixel data in
> GL_RGBA/GL_UNSIGNED_INT. In this case, the destination format matches one of
> these fast path (RGBA UINT) but the source format, which is the render
> buffer's format, is MESA_FORMAT_B8G8R8A8_UNORM which is not integer.

The reason for this is that the original set of [un]packing functions only included functions for [un]packing integer types to/from uint and [un]packing normalized types to/from ubyte.  Any normalized type can (in theory) be converted to a float.  The ubyte versions existed to speed things up when the format was small enough that [un]packing to/from ubyte wouldn't lose any precision.  In theory, we could also do [un]packing to/from 16 and 32-bit normalized types and that would be slightly faster than going through float.  I didn't do this for a couple of reasons

1) Even though autogenerated functions are cheap in the sense that they require almost no effort to code, they do increase the size of the library.  If every format had 4 packing and 4 unpacking functions, that makes for a fairly large amount of code.

2) Most packed unorm formats have fewer than 8 bits per pixel.  The only exception I can think of off hand is 10-10-10-2.  Therefore, most unorm conversions can either go through ubyte, will hit the swizzle_and_convert path, or will have to go through float anyway.  I don't think we really gain much by having them go through uint

3) For integer types, GL only allows conversion to/from float and to/from other integer (not unorm) types.  Since all integer types can fit in 32 bits, uint32 is a natural intermediate format (signed integers get sign-extended).

4) Even moreso than with unorm, the vast majority of integer conversions will go through mesa_swizzle_and_convert.  I belive the only packed integer format is 10-10-10-2.  We need to have the unpacking functions for the odd conversion case, but most won't hit it so also allowing packing to/from lower numbers of bits doesn't gain us much.

> I think we have at least two options:
> 
> 1) Transform the asserts into conditionals. With these we make sure that
> these fast paths are only used when the assertions you have are not
> violated, but we still let _mesa_format_convert use the non-fast paths to
> resolve the conversion in the cases that would hit the assertions. If there
> was a good reason for these assertions to exist then I guess this is the way
> to go.

I think this is the one we want, but I'm not 100% sure I understand what you mean.

> 2) We simply remove the asserts. I think pack/unpack functions can deal with
> any format type, right? At the moment uint pack/unpack functions seem to be
> an exception since they only support packing to and unpacking from integer
> types, but since these functions are auto-generated it seems we could just
> let them handle other types too. Since in this case we would enable the fast
> path, it would be the most efficient solution.
> 
> What do you think?

Given what I said above, does it make more sense now?  It's quite possible that I've missed something, but that explains my reasoning.

(In reply to Samuel Iglesias from comment #36)
> (In reply to Jason Ekstrand from comment #32)
> > (In reply to Iago Toral from comment #31)
> > > Jason, there is an had-hoc path to deal with MESA_FORMAT_YCBCR* in
> > > texstore.c. Seems that this path only allows conversion between these
> > > formats, which is mostly a memcpy with some byte swapping when necessary.
> > > Should we also keep this one as a special case to be handled outside the
> > > master function?
> > 
> > Yes, there's code for that in texstore.c and in format_unpack.c.  We should
> > make that consisten either by movint the code from texstore.c to
> > format_pack.c or move it from format_unpack.c to readpixels.  I'm ok with
> > moving everything into the packing/unpacking functions and letting the
> > conversion function handle it.  It is a MESA_FORMAT after all.
> 
> Looking at _mesa_texstore_ycbcr() inside texstore.c, it is just a memcpy
> without any RGB to YCBCR conversion.
> 
> Do we need to implement RGB to YCBCR conversion in format_pack? Or we just
> need to use a special case for that format in _mesa_format_convert (so that
> it uses the function that memcopies instead of pack/unpack)? The second
> option would be more efficient.

Right... I incorrectly remembered that function doing more than it does.  In that case, let's go ahead and leave it more-or-less as is.  Keep handling it in texstore.  If we leave the unpacking function in there, then GetTexImage and swrast sampling will work, and that should be all we need.

(In reply to Jason Ekstrand from comment #37)
> (In reply to Iago Toral from comment #35)
> > Jason, you wrote some fast paths in _mesa_format_convert for the cases where
> > we can directly pack or unpack to the destination format. In these paths, if
> > we have to pack from or unpack to RGBA8888 UINT, you have asserts to make
> > sure that we are packing to or unpacking from an integer type. Likewise, if
> > we are packing from or unpacking to RGBA8888 UBYTE, you added asserts to
> > make sure that we are packing to or unpacking from a non-integer type.
> > 
> > I am not sure why these were added, but I think we should remove them since
> > there are valid conversions that will hit these paths. For example,
> > glReadPixels where we want to store the pixel data in
> > GL_RGBA/GL_UNSIGNED_INT. In this case, the destination format matches one of
> > these fast path (RGBA UINT) but the source format, which is the render
> > buffer's format, is MESA_FORMAT_B8G8R8A8_UNORM which is not integer.
> 
> The reason for this is that the original set of [un]packing functions only
> included functions for [un]packing integer types to/from uint and
> [un]packing normalized types to/from ubyte.  Any normalized type can (in
> theory) be converted to a float.  The ubyte versions existed to speed things
> up when the format was small enough that [un]packing to/from ubyte wouldn't
> lose any precision.  In theory, we could also do [un]packing to/from 16 and
> 32-bit normalized types and that would be slightly faster than going through
> float.  I didn't do this for a couple of reasons
> 
> 1) Even though autogenerated functions are cheap in the sense that they
> require almost no effort to code, they do increase the size of the library. 
> If every format had 4 packing and 4 unpacking functions, that makes for a
> fairly large amount of code.
> 
> 2) Most packed unorm formats have fewer than 8 bits per pixel.  The only
> exception I can think of off hand is 10-10-10-2.  Therefore, most unorm
> conversions can either go through ubyte, will hit the swizzle_and_convert
> path, or will have to go through float anyway.  I don't think we really gain
> much by having them go through uint
> 
> 3) For integer types, GL only allows conversion to/from float and to/from
> other integer (not unorm) types.  Since all integer types can fit in 32
> bits, uint32 is a natural intermediate format (signed integers get
> sign-extended).
> 
> 4) Even moreso than with unorm, the vast majority of integer conversions
> will go through mesa_swizzle_and_convert.  I belive the only packed integer
> format is 10-10-10-2.  We need to have the unpacking functions for the odd
> conversion case, but most won't hit it so also allowing packing to/from
> lower numbers of bits doesn't gain us much.

I see. I think the problem here is that we check for pack/unpack fast paths before we try _mesa_swizzle_and_convert, so when either the source or the destination are RGBA ubyte/uint/float we will always try the direct pack/unpack path.

I understand this makes sense since directly packing or unpacking should be slightly more efficient if we can do it, but if we do not want to have pack/unpack conversion functions for all types, then we need to make sure that in some cases we fall back to other paths (maybe _mesa_swizzle_and_convert if they are array types) instead of asserting. That would be solution 1) below.

> > I think we have at least two options:
> > 
> > 1) Transform the asserts into conditionals. With these we make sure that
> > these fast paths are only used when the assertions you have are not
> > violated, but we still let _mesa_format_convert use the non-fast paths to
> > resolve the conversion in the cases that would hit the assertions. If there
> > was a good reason for these assertions to exist then I guess this is the way
> > to go.
> 
> I think this is the one we want, but I'm not 100% sure I understand what you
> mean.

From your explanations I think this is what we should do too. For example, when the source type is RGBA ubyte, we will do something like this:

if (src_array_format.as_uint == RGBA8888_UBYTE.as_uint &&
    !_mesa_is_format_integer_color(dst_format) {
   // Call pack function here
}

instead of:

if (src_array_format.as_uint == RGBA8888_UBYTE.as_uint) {
    assert(!_mesa_is_format_integer_color(dst_format));
   // Call pack function here
}

That way, if dst is an integer type we will fall back to _mesa_swizzle_and_convert (if both src and dst are array types) or, as last resort, to the src->rgba->dst conversion path.

> > 2) We simply remove the asserts. I think pack/unpack functions can deal with
> > any format type, right? At the moment uint pack/unpack functions seem to be
> > an exception since they only support packing to and unpacking from integer
> > types, but since these functions are auto-generated it seems we could just
> > let them handle other types too. Since in this case we would enable the fast
> > path, it would be the most efficient solution.
> > 
> > What do you think?
> 
> Given what I said above, does it make more sense now?  It's quite possible
> that I've missed something, but that explains my reasoning.

I think so, thanks for the detailed explanation. I had not considered the increased size of the library if we auto-generate uint pack/unpack functions and was wondering why we would not do that. Also, it is true that in most cases we should be able to handle integer types via swizzle and convert, we just need to make sure that we fall back to that path.

Jason, I think we may want to be a bit more conservative with the pack/unpack fast paths when we consider the internal base format.

Right now, if the src or dst formats are RGBA we will try the pack/unpack fast paths. However, these paths do not consider the internal base format and we can run into cases where simply packing or unpacking is not enough.

Consider this example: the client executes a texture upload of RGBA data and selects a Luminance format for the texture. The driver does not support the luminance format, so decides to use BGRA instead. Since the source format is RGBA, we will directly pack to BGRA, but since the internal base format is luminance, that won't match the semantics we expect (luminance requires to copy R on all components).

I could try to add conditions to protect these paths in situations like this. For example, in this particular case, I could check if the base format of the destination matches the base internal format provided by the client, and if they don't and the internal base format is luminance or intensity, then avoid the fast path. However, I think this might hit other formats too, since the problem, in general, is that pack/unpack fast paths do not consider that the dst format and the dst base internal format might be different and that might involve more than a simples pack or unpack to dst, so I am thinking that maybe we want to simply avoid the pack/unpack paths completely when the dst format is not the same as the dst base internal format.

What do you think?

(In reply to Iago Toral from comment #39)
> From your explanations I think this is what we should do too. For example,
> when the source type is RGBA ubyte, we will do something like this:
> 
> if (src_array_format.as_uint == RGBA8888_UBYTE.as_uint &&
>     !_mesa_is_format_integer_color(dst_format) {
>    // Call pack function here
> }
> 
> instead of:
> 
> if (src_array_format.as_uint == RGBA8888_UBYTE.as_uint) {
>     assert(!_mesa_is_format_integer_color(dst_format));
>    // Call pack function here
> }
> 
> That way, if dst is an integer type we will fall back to
> _mesa_swizzle_and_convert (if both src and dst are array types) or, as last
> resort, to the src->rgba->dst conversion path.

Yes, that seems perfectly reasonable.

(In reply to Iago Toral from comment #40)
> Jason, I think we may want to be a bit more conservative with the
> pack/unpack fast paths when we consider the internal base format.
> 
> Right now, if the src or dst formats are RGBA we will try the pack/unpack
> fast paths. However, these paths do not consider the internal base format
> and we can run into cases where simply packing or unpacking is not enough.
> 
> Consider this example: the client executes a texture upload of RGBA data and
> selects a Luminance format for the texture. The driver does not support the
> luminance format, so decides to use BGRA instead. Since the source format is
> RGBA, we will directly pack to BGRA, but since the internal base format is
> luminance, that won't match the semantics we expect (luminance requires to
> copy R on all components).
> 
> I could try to add conditions to protect these paths in situations like
> this. For example, in this particular case, I could check if the base format
> of the destination matches the base internal format provided by the client,
> and if they don't and the internal base format is luminance or intensity,
> then avoid the fast path. However, I think this might hit other formats too,
> since the problem, in general, is that pack/unpack fast paths do not
> consider that the dst format and the dst base internal format might be
> different and that might involve more than a simples pack or unpack to dst,
> so I am thinking that maybe we want to simply avoid the pack/unpack paths
> completely when the dst format is not the same as the dst base internal
> format.
> 
> What do you think?

That seems reasonable.  We can add code to enable the fast path in the special cases where it makes sense later.  Just disabling it unless we're sure it works seems like the right thing to do.

Jason, we are running into some issues when attempting to use _mesa_format_convert for glReadPixels and glGetTexImage.

Generally, one thing that is different in this case is that the current implementation never attempts direct conversions from src to dst (except for a couple of specific cases that had been written ad-hoc).

These functions do the conversion in 3 steps:
1) unpack to RGBA (float or  uint).
2) Rebase (to consider the base format of the source pixel data).
3) Pack to dst (this not only packs, also handles type conversion, transferops, semantics specific to things like luminance formats, type conversions, etc). 

So we are hitting various issues when attempting to replace this logic with _mesa_format_convert:

1) You mentioned that things like transferops should be handled by the client. To achieve  this we have _mesa_apply_rgba_transfer_ops, which requires an RGBA format, so converting (in the client) to RGBA first is required in these cases so they can use this function. I suppose this is okay since this is what the current implementation is doing in all cases right now, with or without transferops.

2) So far we have been focusing on pixel uploads. I mentioned then that we needed to add a new parameter to _mesa_format_convert to consider the base internal format we are converting to. Well, now we have the same situation but with the format we are converting from, and the semantics are different (in the sense that we need to know if we have a base format for the source or the destination in order to know what we need to do, like computing the right swizzle transform). I suppose that we could add another parameter to _mesa_format_convert and all the logic necessary to do the right thing in that case too. This, will complicate the implementation a bit I think, but I guess is the most consistent option. That said, the alternative would be to always transform to RGBA first, then call _mesa_rebase_rgba_* as the current code does... since the current code always convert to RGBA first we would not be losing performance and we would not have to add all the logic to _mesa_format_convert to account for a source base format. It would be less consistent though since _mesa_format_convert would support base formats for the dst but not for the src.

3) Luminance formats have special requirements. A conversion to Luminance from RGBA requires to do L=R+G+B for example. This is something that _mesa_format_convert cannot achieve at the moment, because neither pack/unpack functions nor _mesa_swizzle_and_convert do this kind of operation, so I wonder what is the right thing to do here. We could run another pass that computes these values after the conversion. We could do this inside _mesa_format_convert or in the client, after calling _mesa_format_convert I suppose there are no other options. The current code does another pass specifically for this, right before packing to the dst.

4) Step 3 does a lot of clamping work even after handling transfer ops, (right after packing to the dst). I am not sure that pack/unpack functions and _mesa_swizzle_and_convert will do the thing we need in all cases, but right now I don't have a specific example, so we can burn that bridge once we get there.

(In reply to Iago Toral from comment #43)
(...)
> 3) Luminance formats have special requirements. A conversion to Luminance
> from RGBA requires to do L=R+G+B for example. This is something that
> _mesa_format_convert cannot achieve at the moment, because neither
> pack/unpack functions nor _mesa_swizzle_and_convert do this kind of
> operation, so I wonder what is the right thing to do here. We could run
> another pass that computes these values after the conversion. We could do
> this inside _mesa_format_convert or in the client, after calling
> _mesa_format_convert I suppose there are no other options. The current code
> does another pass specifically for this, right before packing to the dst.

And likewise, a conversion to RGBA from Luminance requires to do R=L,G=0,B=0,A=1, but unpack functions and _mesa_swizzle_and_convert will do R=L,G=L,B=L,A=1, so again we need another pass to correct this to the expected result.

(In reply to Iago Toral from comment #44)
> (In reply to Iago Toral from comment #43)
> (...)
> > 3) Luminance formats have special requirements. A conversion to Luminance
> > from RGBA requires to do L=R+G+B for example. This is something that
> > _mesa_format_convert cannot achieve at the moment, because neither
> > pack/unpack functions nor _mesa_swizzle_and_convert do this kind of
> > operation, so I wonder what is the right thing to do here. We could run
> > another pass that computes these values after the conversion. We could do
> > this inside _mesa_format_convert or in the client, after calling
> > _mesa_format_convert I suppose there are no other options. The current code
> > does another pass specifically for this, right before packing to the dst.
> 
> And likewise, a conversion to RGBA from Luminance requires to do
> R=L,G=0,B=0,A=1, but unpack functions and _mesa_swizzle_and_convert will do
> R=L,G=L,B=L,A=1, so again we need another pass to correct this to the
> expected result.

That one should be easy.  We can just have a fake internal format for RA.

(In reply to Iago Toral from comment #43)
> Jason, we are running into some issues when attempting to use
> _mesa_format_convert for glReadPixels and glGetTexImage.
> 
> Generally, one thing that is different in this case is that the current
> implementation never attempts direct conversions from src to dst (except for
> a couple of specific cases that had been written ad-hoc).
> 
> These functions do the conversion in 3 steps:
> 1) unpack to RGBA (float or  uint).
> 2) Rebase (to consider the base format of the source pixel data).
> 3) Pack to dst (this not only packs, also handles type conversion,
> transferops, semantics specific to things like luminance formats, type
> conversions, etc). 
> 
> So we are hitting various issues when attempting to replace this logic with
> _mesa_format_convert:
> 
> 1) You mentioned that things like transferops should be handled by the
> client. To achieve  this we have _mesa_apply_rgba_transfer_ops, which
> requires an RGBA format, so converting (in the client) to RGBA first is
> required in these cases so they can use this function. I suppose this is
> okay since this is what the current implementation is doing in all cases
> right now, with or without transferops.
> 
> 2) So far we have been focusing on pixel uploads. I mentioned then that we
> needed to add a new parameter to _mesa_format_convert to consider the base
> internal format we are converting to. Well, now we have the same situation
> but with the format we are converting from, and the semantics are different
> (in the sense that we need to know if we have a base format for the source
> or the destination in order to know what we need to do, like computing the
> right swizzle transform). I suppose that we could add another parameter to
> _mesa_format_convert and all the logic necessary to do the right thing in
> that case too. This, will complicate the implementation a bit I think, but I
> guess is the most consistent option. That said, the alternative would be to
> always transform to RGBA first, then call _mesa_rebase_rgba_* as the current
> code does... since the current code always convert to RGBA first we would
> not be losing performance and we would not have to add all the logic to
> _mesa_format_convert to account for a source base format. It would be less
> consistent though since _mesa_format_convert would support base formats for
> the dst but not for the src.
> 
> 3) Luminance formats have special requirements. A conversion to Luminance
> from RGBA requires to do L=R+G+B for example. This is something that
> _mesa_format_convert cannot achieve at the moment, because neither
> pack/unpack functions nor _mesa_swizzle_and_convert do this kind of
> operation, so I wonder what is the right thing to do here. We could run
> another pass that computes these values after the conversion. We could do
> this inside _mesa_format_convert or in the client, after calling
> _mesa_format_convert I suppose there are no other options. The current code
> does another pass specifically for this, right before packing to the dst.
> 
> 4) Step 3 does a lot of clamping work even after handling transfer ops,
> (right after packing to the dst). I am not sure that pack/unpack functions
> and _mesa_swizzle_and_convert will do the thing we need in all cases, but
> right now I don't have a specific example, so we can burn that bridge once
> we get there.

My though there was to do the following (in pseudocode)

if (!has_transfer_ops && format != LUMINANCE_ALPHA) {
    mesa_format_convert(dst, src);
} else {

}

(In reply to Jason Ekstrand from comment #46)
> (In reply to Iago Toral from comment #43)
> > Jason, we are running into some issues when attempting to use
> > _mesa_format_convert for glReadPixels and glGetTexImage.
> > 
> > Generally, one thing that is different in this case is that the current
> > implementation never attempts direct conversions from src to dst (except for
> > a couple of specific cases that had been written ad-hoc).
> > 
> > These functions do the conversion in 3 steps:
> > 1) unpack to RGBA (float or  uint).
> > 2) Rebase (to consider the base format of the source pixel data).
> > 3) Pack to dst (this not only packs, also handles type conversion,
> > transferops, semantics specific to things like luminance formats, type
> > conversions, etc). 
> > 
> > So we are hitting various issues when attempting to replace this logic with
> > _mesa_format_convert:
> > 
> > 1) You mentioned that things like transferops should be handled by the
> > client. To achieve  this we have _mesa_apply_rgba_transfer_ops, which
> > requires an RGBA format, so converting (in the client) to RGBA first is
> > required in these cases so they can use this function. I suppose this is
> > okay since this is what the current implementation is doing in all cases
> > right now, with or without transferops.
> > 
> > 2) So far we have been focusing on pixel uploads. I mentioned then that we
> > needed to add a new parameter to _mesa_format_convert to consider the base
> > internal format we are converting to. Well, now we have the same situation
> > but with the format we are converting from, and the semantics are different
> > (in the sense that we need to know if we have a base format for the source
> > or the destination in order to know what we need to do, like computing the
> > right swizzle transform). I suppose that we could add another parameter to
> > _mesa_format_convert and all the logic necessary to do the right thing in
> > that case too. This, will complicate the implementation a bit I think, but I
> > guess is the most consistent option. That said, the alternative would be to
> > always transform to RGBA first, then call _mesa_rebase_rgba_* as the current
> > code does... since the current code always convert to RGBA first we would
> > not be losing performance and we would not have to add all the logic to
> > _mesa_format_convert to account for a source base format. It would be less
> > consistent though since _mesa_format_convert would support base formats for
> > the dst but not for the src.
> > 
> > 3) Luminance formats have special requirements. A conversion to Luminance
> > from RGBA requires to do L=R+G+B for example. This is something that
> > _mesa_format_convert cannot achieve at the moment, because neither
> > pack/unpack functions nor _mesa_swizzle_and_convert do this kind of
> > operation, so I wonder what is the right thing to do here. We could run
> > another pass that computes these values after the conversion. We could do
> > this inside _mesa_format_convert or in the client, after calling
> > _mesa_format_convert I suppose there are no other options. The current code
> > does another pass specifically for this, right before packing to the dst.
> > 
> > 4) Step 3 does a lot of clamping work even after handling transfer ops,
> > (right after packing to the dst). I am not sure that pack/unpack functions
> > and _mesa_swizzle_and_convert will do the thing we need in all cases, but
> > right now I don't have a specific example, so we can burn that bridge once
> > we get there.
> 
> My though there was to do the following (in pseudocode)
> 
> if (!has_transfer_ops && format != LUMINANCE_ALPHA) {
>     mesa_format_convert(dst, src);
> } else {
> 
> }

Yes, this is what we have done but I was wondering if we could just handle conversion to luminance inside _mesa_format_convert as a special case to avoid having that if { } else {} in every path that implements format conversion (glTexImage, glGetTexImage, glReadPIxels, etc)... then I noticed that for _mesa_format_convert to do that it would have to know about transferops or at least have a way to know if we have to clamp the result, so it is probably not a good idea.

(In reply to Jason Ekstrand from comment #34)
> (In reply to Samuel Iglesias from comment #33)
> > Jason, I would like to know your opinion about the integer RGBA clamping
> > done in pack.c (_mesa_pack_rgba_span_from_ints()).
> > 
> > glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3.
> > core specification) that for an integer RGBA color, each component is
> > clamped to the representable range of type.
> > 
> > Those GL functions end up calling pack.c's functions for performing the
> > conversion (mesa_pack_rgba_span_from_ints() and others).
> > 
> > It's possible to replace some of those pack.c's conversion functions by the
> > master conversion but the problem is in the clamping operation. I would like
> > to add a boolean argument called "clamp" to the master conversion function
> > which is passed to _mesa_swizzle_and_convert() where each of its
> > convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp"
> > is true.
> > 
> > This "clamp" parameter would be false for every call to either master
> > conversion function or _mesa_swizzle_and_convert() except when they are
> > being called from pack.c.
> > 
> > What do you think?
> 
> Supporting clamping is probably fine.  I think we determined we needed a
> clamp parameter anyway for some of the float conversions.  I guess it makes
> sense for integers too.  Let's watch out for performance when we implement
> it though.  Changing the loop inside mesa_swizzle_and_convert without
> hurting performance can be tricky.  The teximage-colors test in Piglit has a
> -benchmark flag that can be used for testing that.

In the end, we did not make that change in pack.c as we could just use the autogenerated format pack/unpack functions. However I am retaking this topic again because we found another issue which would require a similar solution:

The convert_*() functions in format_utils.c convert between source and destination data and are used by _mesa_swizzle_and_convert. We found that these were not good enough for various conversions that involved non-normalized types of different sizes: INT to SHORT, INT to BYTE, etc. Because of that, several piglit tests related to glGetTexImage() and others failed, like for example bin/ext_texture_integer-getteximage-clamping.

In order to fix that we added the clamp expressions for these cases [1]  and with that we achieved no regressions when executing a full piglit run on i965 driver.
Unfortunately, when testing our patches on a couple of Gallium drivers, we found a regression that we tracked down back to that patch: bin/arb_clear_buffer_object-formats.
Reverting the patch makes fixes the problem with these Gallium drivers but then, bin/ext_texture_integer-getteximage-clamping fails again on i965. We wonder if there could be more cases like this that piglit is not covering, since it looks weird that this affects just this one test.

So, we wonder if that patch for _mesa_swizzle_and_convert is correct and we should fix the failed gallium cases elsewhere, or if we should revert that patch and fix the cases it fixed in a different way. What do you think? Was _mesa_swizzle_and_convert implemented like that  on purpose or are these real bugs?

If we decide to revert our clamp patch, then a solution could be to have a separate implementation of _mesa_swizzle_and_convert() and its convert_*() functions that clamps. We would have to use that version in glGetTexImage() (maybe in glReadPIxels too)  and use the normal version for texture uploads (texstore). If we do this, then we would have to add a clamp flag to _mesa_format_convert, the same flag to _mesa_swizzle_and_convert and the have a new collection of convert_*_clamp() functions for the sake of performance (so we don't have to check whether we have to clamp or not for each pixel we process).  We have tested this already and it works fine (no regressions) for i965. We also repeated the full piglit run on Gallium 0.4 on AMD RV710 and it showed zero regressions. And the benchmark you mention in the previous comment showed that there weren't significative differences in the total time of both runs (with and without the patch).

What do you think? Should we keep the clamp patch? Would you suggest a different approach?

[1] http://pastebin.com/CPYbCU8e

Jason you had this commit in your original branch: "MAYBEREVERT: Fill X components with 1"

That basically makes packing to padded formats (like RGBX) set X=1. In the commit message you mention that you are not sure you like this... my opinion is that this should not be necessary and it degrades performance when packing to these format, so I would be more for removing this one. What do you think?

FWIW, I have checked that removing this commit does not affect piglit results.

(In reply to Iago Toral from comment #49)
> Jason you had this commit in your original branch: "MAYBEREVERT: Fill X
> components with 1"
> 
> That basically makes packing to padded formats (like RGBX) set X=1. In the
> commit message you mention that you are not sure you like this... my opinion
> is that this should not be necessary and it degrades performance when
> packing to these format, so I would be more for removing this one. What do
> you think?
> 
> FWIW, I have checked that removing this commit does not affect piglit
> results.

Go ahead and remove it.

(In reply to Samuel Iglesias from comment #48)
> (In reply to Jason Ekstrand from comment #34)
> > (In reply to Samuel Iglesias from comment #33)
> > > Jason, I would like to know your opinion about the integer RGBA clamping
> > > done in pack.c (_mesa_pack_rgba_span_from_ints()).
> > > 
> > > glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3.
> > > core specification) that for an integer RGBA color, each component is
> > > clamped to the representable range of type.
> > > 
> > > Those GL functions end up calling pack.c's functions for performing the
> > > conversion (mesa_pack_rgba_span_from_ints() and others).
> > > 
> > > It's possible to replace some of those pack.c's conversion functions by the
> > > master conversion but the problem is in the clamping operation. I would like
> > > to add a boolean argument called "clamp" to the master conversion function
> > > which is passed to _mesa_swizzle_and_convert() where each of its
> > > convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp"
> > > is true.
> > > 
> > > This "clamp" parameter would be false for every call to either master
> > > conversion function or _mesa_swizzle_and_convert() except when they are
> > > being called from pack.c.
> > > 
> > > What do you think?
> > 
> > Supporting clamping is probably fine.  I think we determined we needed a
> > clamp parameter anyway for some of the float conversions.  I guess it makes
> > sense for integers too.  Let's watch out for performance when we implement
> > it though.  Changing the loop inside mesa_swizzle_and_convert without
> > hurting performance can be tricky.  The teximage-colors test in Piglit has a
> > -benchmark flag that can be used for testing that.
> 
> In the end, we did not make that change in pack.c as we could just use the
> autogenerated format pack/unpack functions. However I am retaking this topic
> again because we found another issue which would require a similar solution:
> 
> The convert_*() functions in format_utils.c convert between source and
> destination data and are used by _mesa_swizzle_and_convert. We found that
> these were not good enough for various conversions that involved
> non-normalized types of different sizes: INT to SHORT, INT to BYTE, etc.
> Because of that, several piglit tests related to glGetTexImage() and others
> failed, like for example bin/ext_texture_integer-getteximage-clamping.
> 
> In order to fix that we added the clamp expressions for these cases [1]  and
> with that we achieved no regressions when executing a full piglit run on
> i965 driver.
> Unfortunately, when testing our patches on a couple of Gallium drivers, we
> found a regression that we tracked down back to that patch:
> bin/arb_clear_buffer_object-formats.
> Reverting the patch makes fixes the problem with these Gallium drivers but
> then, bin/ext_texture_integer-getteximage-clamping fails again on i965. We
> wonder if there could be more cases like this that piglit is not covering,
> since it looks weird that this affects just this one test.
> 
> So, we wonder if that patch for _mesa_swizzle_and_convert is correct and we
> should fix the failed gallium cases elsewhere, or if we should revert that
> patch and fix the cases it fixed in a different way. What do you think? Was
> _mesa_swizzle_and_convert implemented like that  on purpose or are these
> real bugs?

From my brief reading of the GL spec, it looks like clamping integers to the max representable range is what it expects by default.  From the glTexImage spec:

"The selected groups are transferred to the GL as described in section 3.7.2 and then clamped to the representable range of the internal format. If the
internalformat of the texture is signed or unsigned integer, components are clamped to [-2^(n-1), 2^(n-1)-1] or [0, 2^(n-1)-1], respectively, where n is the number of bits per component. For color component groups, if the internalformat of the texture is signed or unsigned normalized fixed-point, components are clamped t0 [-1, 1] or [0, 1], respectively."

Therefore, it seems as if we want to be clamping when we have integer destinations.  I'm not sure why the gallium drivers are regressing when you do.

One more observation is that it doesn't look like your clamping patch is complete.  If we're going to clamp when we have an integer destination, we should always clamp with integer destinations, not just in the few cases that piglit hits.  I wouldn't be surprised if piglit's coverage in those areas is terrible.

> If we decide to revert our clamp patch, then a solution could be to have a
> separate implementation of _mesa_swizzle_and_convert() and its convert_*()
> functions that clamps. We would have to use that version in glGetTexImage()
> (maybe in glReadPIxels too)  and use the normal version for texture uploads
> (texstore). If we do this, then we would have to add a clamp flag to
> _mesa_format_convert, the same flag to _mesa_swizzle_and_convert and the
> have a new collection of convert_*_clamp() functions for the sake of
> performance (so we don't have to check whether we have to clamp or not for
> each pixel we process).  We have tested this already and it works fine (no
> regressions) for i965. We also repeated the full piglit run on Gallium 0.4
> on AMD RV710 and it showed zero regressions. And the benchmark you mention
> in the previous comment showed that there weren't significative differences
> in the total time of both runs (with and without the patch).

At this point, it seems like a little more digging into what's happening on your RV710 would be good.

(In reply to Jason Ekstrand from comment #51)
> (In reply to Samuel Iglesias from comment #48)
> > (In reply to Jason Ekstrand from comment #34)
> > > (In reply to Samuel Iglesias from comment #33)
> > > > Jason, I would like to know your opinion about the integer RGBA clamping
> > > > done in pack.c (_mesa_pack_rgba_span_from_ints()).
> > > > 
> > > > glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3.
> > > > core specification) that for an integer RGBA color, each component is
> > > > clamped to the representable range of type.
> > > > 
> > > > Those GL functions end up calling pack.c's functions for performing the
> > > > conversion (mesa_pack_rgba_span_from_ints() and others).
> > > > 
> > > > It's possible to replace some of those pack.c's conversion functions by the
> > > > master conversion but the problem is in the clamping operation. I would like
> > > > to add a boolean argument called "clamp" to the master conversion function
> > > > which is passed to _mesa_swizzle_and_convert() where each of its
> > > > convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp"
> > > > is true.
> > > > 
> > > > This "clamp" parameter would be false for every call to either master
> > > > conversion function or _mesa_swizzle_and_convert() except when they are
> > > > being called from pack.c.
> > > > 
> > > > What do you think?
> > > 
> > > Supporting clamping is probably fine.  I think we determined we needed a
> > > clamp parameter anyway for some of the float conversions.  I guess it makes
> > > sense for integers too.  Let's watch out for performance when we implement
> > > it though.  Changing the loop inside mesa_swizzle_and_convert without
> > > hurting performance can be tricky.  The teximage-colors test in Piglit has a
> > > -benchmark flag that can be used for testing that.
> > 
> > In the end, we did not make that change in pack.c as we could just use the
> > autogenerated format pack/unpack functions. However I am retaking this topic
> > again because we found another issue which would require a similar solution:
> > 
> > The convert_*() functions in format_utils.c convert between source and
> > destination data and are used by _mesa_swizzle_and_convert. We found that
> > these were not good enough for various conversions that involved
> > non-normalized types of different sizes: INT to SHORT, INT to BYTE, etc.
> > Because of that, several piglit tests related to glGetTexImage() and others
> > failed, like for example bin/ext_texture_integer-getteximage-clamping.
> > 
> > In order to fix that we added the clamp expressions for these cases [1]  and
> > with that we achieved no regressions when executing a full piglit run on
> > i965 driver.
> > Unfortunately, when testing our patches on a couple of Gallium drivers, we
> > found a regression that we tracked down back to that patch:
> > bin/arb_clear_buffer_object-formats.
> > Reverting the patch makes fixes the problem with these Gallium drivers but
> > then, bin/ext_texture_integer-getteximage-clamping fails again on i965. We
> > wonder if there could be more cases like this that piglit is not covering,
> > since it looks weird that this affects just this one test.
> > 
> > So, we wonder if that patch for _mesa_swizzle_and_convert is correct and we
> > should fix the failed gallium cases elsewhere, or if we should revert that
> > patch and fix the cases it fixed in a different way. What do you think? Was
> > _mesa_swizzle_and_convert implemented like that  on purpose or are these
> > real bugs?
> 
> From my brief reading of the GL spec, it looks like clamping integers to the
> max representable range is what it expects by default.  From the glTexImage
> spec:
> 
> "The selected groups are transferred to the GL as described in section 3.7.2
> and then clamped to the representable range of the internal format. If the
> internalformat of the texture is signed or unsigned integer, components are
> clamped to [-2^(n-1), 2^(n-1)-1] or [0, 2^(n-1)-1], respectively, where n is
> the number of bits per component. For color component groups, if the
> internalformat of the texture is signed or unsigned normalized fixed-point,
> components are clamped t0 [-1, 1] or [0, 1], respectively."
> 
> Therefore, it seems as if we want to be clamping when we have integer
> destinations.  I'm not sure why the gallium drivers are regressing when you
> do.
> 
> One more observation is that it doesn't look like your clamping patch is
> complete.  If we're going to clamp when we have an integer destination, we
> should always clamp with integer destinations, not just in the few cases
> that piglit hits.  I wouldn't be surprised if piglit's coverage in those
> areas is terrible.
>

Yes, you are right about the specification. We have added clamping for all the needed conversions in _mesa_swizzle_and_convert() following the spec but, after analyzing why gallium drivers have this regression, I discovered that it comes from other place.

The following text was copied from piglit's arb_clear_buffer_object-formats program source code explaining its purpose:

   Test clearing the entire buffer with multiple internal formats. Pass the data
   to clear the buffer with in a format so that the GL doesn't have to do any
   format conversion.

When "bin/arb_clear_buffer_object-formats" program was testing glClearBufferData() with GL_LUMINANCE_ALPHA16I_EXT and GL_LUMINANCE_ALPHA32I_EXT internal formats, they were not assigned to their respective MESA_FORMAT_LA_SINT16 and MESA_FORMAT_LA_SINT32 mesa formats but to MESA_FORMAT_LA_SINT8 and MESA_FORMAT_LA_SINT16 in get_texbuffer_format().

Then memcpy path was not executed, texstore_rgba() was called instead and, as we added clamping in _mesa_swizzle_and_convert(), the bug appeared. I wrote a patch fixing that function and now everything is working as expected.

This program don't test these internal formats in i965 because they are not part of the core profile. So we had this bug there without noticing.

Thanks for your comments.

(In reply to Samuel Iglesias from comment #52)
> (In reply to Jason Ekstrand from comment #51)
> > (In reply to Samuel Iglesias from comment #48)
> > > (In reply to Jason Ekstrand from comment #34)
> > > > (In reply to Samuel Iglesias from comment #33)
> > > > > Jason, I would like to know your opinion about the integer RGBA clamping
> > > > > done in pack.c (_mesa_pack_rgba_span_from_ints()).
> > > > > 
> > > > > glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3.
> > > > > core specification) that for an integer RGBA color, each component is
> > > > > clamped to the representable range of type.
> > > > > 
> > > > > Those GL functions end up calling pack.c's functions for performing the
> > > > > conversion (mesa_pack_rgba_span_from_ints() and others).
> > > > > 
> > > > > It's possible to replace some of those pack.c's conversion functions by the
> > > > > master conversion but the problem is in the clamping operation. I would like
> > > > > to add a boolean argument called "clamp" to the master conversion function
> > > > > which is passed to _mesa_swizzle_and_convert() where each of its
> > > > > convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp"
> > > > > is true.
> > > > > 
> > > > > This "clamp" parameter would be false for every call to either master
> > > > > conversion function or _mesa_swizzle_and_convert() except when they are
> > > > > being called from pack.c.
> > > > > 
> > > > > What do you think?
> > > > 
> > > > Supporting clamping is probably fine.  I think we determined we needed a
> > > > clamp parameter anyway for some of the float conversions.  I guess it makes
> > > > sense for integers too.  Let's watch out for performance when we implement
> > > > it though.  Changing the loop inside mesa_swizzle_and_convert without
> > > > hurting performance can be tricky.  The teximage-colors test in Piglit has a
> > > > -benchmark flag that can be used for testing that.
> > > 
> > > In the end, we did not make that change in pack.c as we could just use the
> > > autogenerated format pack/unpack functions. However I am retaking this topic
> > > again because we found another issue which would require a similar solution:
> > > 
> > > The convert_*() functions in format_utils.c convert between source and
> > > destination data and are used by _mesa_swizzle_and_convert. We found that
> > > these were not good enough for various conversions that involved
> > > non-normalized types of different sizes: INT to SHORT, INT to BYTE, etc.
> > > Because of that, several piglit tests related to glGetTexImage() and others
> > > failed, like for example bin/ext_texture_integer-getteximage-clamping.
> > > 
> > > In order to fix that we added the clamp expressions for these cases [1]  and
> > > with that we achieved no regressions when executing a full piglit run on
> > > i965 driver.
> > > Unfortunately, when testing our patches on a couple of Gallium drivers, we
> > > found a regression that we tracked down back to that patch:
> > > bin/arb_clear_buffer_object-formats.
> > > Reverting the patch makes fixes the problem with these Gallium drivers but
> > > then, bin/ext_texture_integer-getteximage-clamping fails again on i965. We
> > > wonder if there could be more cases like this that piglit is not covering,
> > > since it looks weird that this affects just this one test.
> > > 
> > > So, we wonder if that patch for _mesa_swizzle_and_convert is correct and we
> > > should fix the failed gallium cases elsewhere, or if we should revert that
> > > patch and fix the cases it fixed in a different way. What do you think? Was
> > > _mesa_swizzle_and_convert implemented like that  on purpose or are these
> > > real bugs?
> > 
> > From my brief reading of the GL spec, it looks like clamping integers to the
> > max representable range is what it expects by default.  From the glTexImage
> > spec:
> > 
> > "The selected groups are transferred to the GL as described in section 3.7.2
> > and then clamped to the representable range of the internal format. If the
> > internalformat of the texture is signed or unsigned integer, components are
> > clamped to [-2^(n-1), 2^(n-1)-1] or [0, 2^(n-1)-1], respectively, where n is
> > the number of bits per component. For color component groups, if the
> > internalformat of the texture is signed or unsigned normalized fixed-point,
> > components are clamped t0 [-1, 1] or [0, 1], respectively."
> > 
> > Therefore, it seems as if we want to be clamping when we have integer
> > destinations.  I'm not sure why the gallium drivers are regressing when you
> > do.
> > 
> > One more observation is that it doesn't look like your clamping patch is
> > complete.  If we're going to clamp when we have an integer destination, we
> > should always clamp with integer destinations, not just in the few cases
> > that piglit hits.  I wouldn't be surprised if piglit's coverage in those
> > areas is terrible.
> >
> 
> Yes, you are right about the specification. We have added clamping for all
> the needed conversions in _mesa_swizzle_and_convert() following the spec
> but, after analyzing why gallium drivers have this regression, I discovered
> that it comes from other place.
> 
> The following text was copied from piglit's arb_clear_buffer_object-formats
> program source code explaining its purpose:
> 
>    Test clearing the entire buffer with multiple internal formats. Pass the
> data
>    to clear the buffer with in a format so that the GL doesn't have to do any
>    format conversion.
> 
> When "bin/arb_clear_buffer_object-formats" program was testing
> glClearBufferData() with GL_LUMINANCE_ALPHA16I_EXT and
> GL_LUMINANCE_ALPHA32I_EXT internal formats, they were not assigned to their
> respective MESA_FORMAT_LA_SINT16 and MESA_FORMAT_LA_SINT32 mesa formats but
> to MESA_FORMAT_LA_SINT8 and MESA_FORMAT_LA_SINT16 in get_texbuffer_format().
> 
> Then memcpy path was not executed, texstore_rgba() was called instead and,
> as we added clamping in _mesa_swizzle_and_convert(), the bug appeared. I
> wrote a patch fixing that function and now everything is working as expected.
> 
> This program don't test these internal formats in i965 because they are not
> part of the core profile. So we had this bug there without noticing.
> 
> Thanks for your comments.

Good work!

Hi Jason,

I think we are close to completing this, so here is a summary of the current state and how we intend to submit the patches for review, since we assume that you will be reviewing most of these:

(In reply to Jason Ekstrand from comment #0)
(...)
> I would like to fix clean up this mess and unify a lot of the conversion
> code.  Here's what I've envisioned:
> 
>  1) Autogenerate all of the color packing/unpacking functions

Done.

>  2) Convert all of the packing/unpacking functions to pack/unpack a
> rectangle taking a width, height, and stride.  We can use 1x1 for
> single-pixel conversions.

We decided not to do this yet because in the end format conversion will mostly happen through _mesa_format_convert now which already accepts a rect. Although making the pack/unpack functions accept a rect would slightly simplify _mesa_format_covert I think it won't really make a big difference anywhere else, so we have kept postponing this until now. We can still do this change if you think it is worth it.

>  3) Make swrast use the unpacking functions instead of its own texture
> sampling functions.
>  4) Add an array format enum that allows us to enumerate all possible array
> formats.  Between mesa_format and this array format, we should also be able
> to enumerate all of the GL datatypes.
>  5) Make a masater conversion function that takes a void*, format, width,
> height, and stride for both source and destination and just does the
> conversion.  If the above mentioned array format enum is distinct from the
> mesa_format enum, the function could be written to take a uint32_t type and
> accept either mesa_format or an array format in the same parameter.
>  6) Use the above master conversion function for TexSubimage, TexImage,
> GetTexImage, DrawPixels, and ReadPixels.  We still have to deal with pixel
> conversion, but it should vastly simplify all of them.

All this is done.

We have run piglit for i965, classic swrast, gallium swrast, llvmpipe, nouveau and gallium radeon without regressions so I guess it should be quite okay. Actually, there are now about 14 new piglit subtests that pass when compared to master.

That said, I think there are probably many conversions that piglit won't cover, so I would not be surprised if we have a bunch of regressions anyway, but hopefully they won't be anything major.

We are now preparing the patch set for review now. It is a large collection of patches (>50 patches), so we are thinking to split it in 3 series: one with general bugfixes, another for the auto-generation of pack/unpack functions and a final series with the master convert function.

Git says that the patches add ~5K LOC and remove ~15K LOC, so it a huge cleanup.

(In reply to Iago Toral from comment #54)
> Hi Jason,
> 
> I think we are close to completing this, so here is a summary of the current
> state and how we intend to submit the patches for review, since we assume
> that you will be reviewing most of these:
> 
> (In reply to Jason Ekstrand from comment #0)
> (...)
> > I would like to fix clean up this mess and unify a lot of the conversion
> > code.  Here's what I've envisioned:
> > 
> >  1) Autogenerate all of the color packing/unpacking functions
> 
> Done.
> 
> >  2) Convert all of the packing/unpacking functions to pack/unpack a
> > rectangle taking a width, height, and stride.  We can use 1x1 for
> > single-pixel conversions.
> 
> We decided not to do this yet because in the end format conversion will
> mostly happen through _mesa_format_convert now which already accepts a rect.
> Although making the pack/unpack functions accept a rect would slightly
> simplify _mesa_format_covert I think it won't really make a big difference
> anywhere else, so we have kept postponing this until now. We can still do
> this change if you think it is worth it.

That's fine.  It's not that big of a change.  Mostly, it's just a small performance boost for really narrow conversions.

> >  3) Make swrast use the unpacking functions instead of its own texture
> > sampling functions.
> >  4) Add an array format enum that allows us to enumerate all possible array
> > formats.  Between mesa_format and this array format, we should also be able
> > to enumerate all of the GL datatypes.
> >  5) Make a masater conversion function that takes a void*, format, width,
> > height, and stride for both source and destination and just does the
> > conversion.  If the above mentioned array format enum is distinct from the
> > mesa_format enum, the function could be written to take a uint32_t type and
> > accept either mesa_format or an array format in the same parameter.
> >  6) Use the above master conversion function for TexSubimage, TexImage,
> > GetTexImage, DrawPixels, and ReadPixels.  We still have to deal with pixel
> > conversion, but it should vastly simplify all of them.
> 
> All this is done.
> 
> We have run piglit for i965, classic swrast, gallium swrast, llvmpipe,
> nouveau and gallium radeon without regressions so I guess it should be quite
> okay. Actually, there are now about 14 new piglit subtests that pass when
> compared to master.

Good work!  If it passes all of those, then I'm not too concerned about the kinds of hidden bugs we hit on my first format conversion series.

> That said, I think there are probably many conversions that piglit won't
> cover, so I would not be surprised if we have a bunch of regressions anyway,
> but hopefully they won't be anything major.

I'm sure there's things piglit covers.  However, I'm going to give it a good look over and between doing what "looks right" and passing piglit, I think we've got it mostly covered.

> We are now preparing the patch set for review now. It is a large collection
> of patches (>50 patches), so we are thinking to split it in 3 series: one
> with general bugfixes, another for the auto-generation of pack/unpack
> functions and a final series with the master convert function.

That seems perfectly reasonable.

> Git says that the patches add ~5K LOC and remove ~15K LOC, so it a huge
> cleanup.

Nice!

This was merged as of a few weeks ago.  Good work guys!