On Thu, 2014-11-20 at 10:49 -0800, Jason Ekstrand wrote: > > > On Wed, Nov 19, 2014 at 11:58 PM, Iago Toral <ito...@igalia.com> > wrote: > On Wed, 2014-11-19 at 11:57 -0800, Jason Ekstrand wrote: > > A couple of general comments on this patch: > > > > > > 1) The prerequisites should be moved to before the first > patch in the > > series and it should be squashed into the patch that > introduces the > > function. There are one or two more patches which also > modify it and > > those should also be squashed in. > > Ok. > > > > > 2) I wonder if giving _mesa_format_convert an internal > swizzle > > wouldn't be better than a destination internal format. > There are a > > couple of reasons for this: > > > > a) It's more general. If it doesn't cost us anything > extra to do > > it that way, we might as well. > > I think that would only work directly for conversions between > array > formats, but what if we have, for example, a texture upload > from RGB565 > to a Luminance format (so that we get an RGBA base format)? > that would > not go though _mesa_swizzle_and_convert and would require to > unpack to > RGBA and then pack to the dst... and unless the client has > provided the > dst format as an array format that won't use > _mesa_swizzle_and_convert > either. That should not be a problem when the calls to > _mesa_format_convert come from things like glTexImage or > glReadPixels, > since in these cases the compute the dst format from a GL type > and if it > is an array format we should get that, but in other cases that > might not > be the case... > > > I'm a bit confused about what you mean here. If the user passes in a > non-trivial swizzle and we have to pack and unpack on both sides then > we have to unpack, swizzle, and then repack. We would still have to > do this if all you pass in is an internal format.
I was confused by the fact that we are currently not doing this in all paths (I mean, swizzling after unpacking if necessary), but you are right. > Fortunately, the _mesa_swizzle_and_convert function can be used to do > an in-place swizzle as long as the source and destination types have > the same number of bits per pixel. > > If one side of the pack/repack is an array format, we can just build > the swizzling into the one _mesa_swizzle_and_convert call. Yes, I get your point now. Thanks! > > > b) It removes the GL enum dependency from the > _mesa_format_convert > > c) I think this implementation misses the case where we > download a > > texture that is storred in a different format than its base > format. > > For instance, if you are downloading a GL_RGB texture as > GL_RGBA but > > it's storred as GL_RGBA. I think with the current > implementaion, > > you'll get the junk in the alpha component of the texture's > backing > > storage instead of a constant alpha value of 1. > > That's correct. In the implementation of readpixels and > getteximage we > had to rebase the results in some cases to account for that. > > Iago > > > > > On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga > > <ito...@igalia.com> wrote: > > In general, if the dst base internal format and the > selected > > dst format are > > different we can't be sure that directly packing or > unpacking > > to the destination > > format will produce correct results. In these cases > we > > probably want to fall > > back to other paths (like mesa_swizzle_and_convert) > that can > > handle these > > situations better. > > > > An example of this includes a luminance internal > format for > > which the driver > > decided to use something like BGRA. In these case, > unpacking > > to BGRA won't > > match the requirements of the luminance internal > format. > > > > In the future we may want to allow these fast paths > for > > specific cases > > where we know the direct pack/unpack functions will > do the > > right thing. > > --- > > src/mesa/main/format_utils.c | 137 > > +++++++++++++++++++++++-------------------- > > 1 file changed, 72 insertions(+), 65 deletions(-) > > > > diff --git a/src/mesa/main/format_utils.c > > b/src/mesa/main/format_utils.c > > index 5964689..34c90d9 100644 > > --- a/src/mesa/main/format_utils.c > > +++ b/src/mesa/main/format_utils.c > > @@ -331,65 +331,82 @@ _mesa_format_convert(void > *void_dst, > > uint32_t dst_format, size_t dst_stride, > > dst_array_format.as_uint = > > _mesa_format_to_array_format(dst_format); > > } > > > > - /* Handle the cases where we can directly unpack > */ > > - if (!(src_format & MESA_ARRAY_FORMAT_BIT)) { > > - if (dst_array_format.as_uint == > RGBA8888_FLOAT.as_uint) > > { > > - for (row = 0; row < height; ++row) { > > - _mesa_unpack_rgba_row(src_format, > width, > > - src, (float > (*)[4])dst); > > - src += src_stride; > > - dst += dst_stride; > > - } > > - return; > > - } else if (dst_array_format.as_uint == > > RGBA8888_UBYTE.as_uint && > > - ! > _mesa_is_format_integer_color(src_format)) > > { > > - for (row = 0; row < height; ++row) { > > - _mesa_unpack_ubyte_rgba_row(src_format, > width, > > - src, > (uint8_t > > (*)[4])dst); > > - src += src_stride; > > - dst += dst_stride; > > - } > > - return; > > - } else if (dst_array_format.as_uint == > > RGBA8888_UINT.as_uint && > > - > _mesa_is_format_integer_color(src_format)) { > > - for (row = 0; row < height; ++row) { > > - _mesa_unpack_uint_rgba_row(src_format, > width, > > - src, > (uint32_t > > (*)[4])dst); > > - src += src_stride; > > - dst += dst_stride; > > + /* First we see if we can implement the > conversion with a > > direct pack > > + * or unpack. > > + * > > + * In this case we want to be careful when the > dst base > > format and the > > + * dst format do not match. In these cases a > simple > > pack/unpack to the > > + * dst format from the src format may not match > the > > semantic requirements > > + * of the internal base format. For now we > decide to be > > safe and avoid > > + * this path in these scenarios but in the > future we may > > want to enable > > + * it for specific combinations that are known > to work. > > + */ > > + mesa_format dst_mesa_format; > > + if (dst_format & MESA_ARRAY_FORMAT_BIT) > > + dst_mesa_format = > > _mesa_format_from_array_format(dst_format); > > + else > > + dst_mesa_format = dst_format; > > + if > (_mesa_get_format_base_format(dst_mesa_format) == > > dst_internal_format) { > > + /* Handle the cases where we can directly > unpack */ > > + if (!(src_format & MESA_ARRAY_FORMAT_BIT)) { > > + if (dst_array_format.as_uint == > > RGBA8888_FLOAT.as_uint) { > > + for (row = 0; row < height; ++row) { > > + _mesa_unpack_rgba_row(src_format, > width, > > + src, (float > (*)[4])dst); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > + } else if (dst_array_format.as_uint == > > RGBA8888_UBYTE.as_uint && > > + ! > > > _mesa_is_format_integer_color(src_format)) { > > + for (row = 0; row < height; ++row) { > > + > _mesa_unpack_ubyte_rgba_row(src_format, width, > > + src, > (uint8_t > > (*)[4])dst); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > + } else if (dst_array_format.as_uint == > > RGBA8888_UINT.as_uint && > > + > > _mesa_is_format_integer_color(src_format)) { > > + for (row = 0; row < height; ++row) { > > + > _mesa_unpack_uint_rgba_row(src_format, width, > > + src, > (uint32_t > > (*)[4])dst); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > } > > - return; > > } > > - } > > > > - /* Handle the cases where we can directly pack > */ > > - if (!(dst_format & MESA_ARRAY_FORMAT_BIT)) { > > - if (src_array_format.as_uint == > RGBA8888_FLOAT.as_uint) > > { > > - for (row = 0; row < height; ++row) { > > - _mesa_pack_float_rgba_row(dst_format, > width, > > - (const float > > (*)[4])src, dst); > > - src += src_stride; > > - dst += dst_stride; > > - } > > - return; > > - } else if (src_array_format.as_uint == > > RGBA8888_UBYTE.as_uint && > > - ! > _mesa_is_format_integer_color(dst_format)) > > { > > - for (row = 0; row < height; ++row) { > > - _mesa_pack_ubyte_rgba_row(dst_format, > width, > > - (const > uint8_t > > (*)[4])src, dst); > > - src += src_stride; > > - dst += dst_stride; > > - } > > - return; > > - } else if (src_array_format.as_uint == > > RGBA8888_UINT.as_uint && > > - > _mesa_is_format_integer_color(dst_format)) { > > - for (row = 0; row < height; ++row) { > > - _mesa_pack_uint_rgba_row(dst_format, > width, > > - (const > uint32_t > > (*)[4])src, dst); > > - src += src_stride; > > - dst += dst_stride; > > + /* Handle the cases where we can directly > pack. */ > > + if (!(dst_format & MESA_ARRAY_FORMAT_BIT)) { > > + if (src_array_format.as_uint == > > RGBA8888_FLOAT.as_uint) { > > + for (row = 0; row < height; ++row) { > > + > _mesa_pack_float_rgba_row(dst_format, width, > > + (const > float > > (*)[4])src, dst); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > + } else if (src_array_format.as_uint == > > RGBA8888_UBYTE.as_uint && > > + ! > > > _mesa_is_format_integer_color(dst_format)) { > > + for (row = 0; row < height; ++row) { > > + > _mesa_pack_ubyte_rgba_row(dst_format, width, > > + (const > uint8_t > > (*)[4])src, dst); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > + } else if (src_array_format.as_uint == > > RGBA8888_UINT.as_uint && > > + > > _mesa_is_format_integer_color(dst_format)) { > > + for (row = 0; row < height; ++row) { > > + _mesa_pack_uint_rgba_row(dst_format, > width, > > + (const > uint32_t > > (*)[4])src, dst); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > } > > - return; > > } > > } > > > > @@ -431,11 +448,6 @@ _mesa_format_convert(void > *void_dst, > > uint32_t dst_format, size_t dst_stride, > > * by computing the swizzle transform > > src->rgba->base->rgba->dst instead > > * of src->rgba->dst. > > */ > > - mesa_format dst_mesa_format; > > - if (dst_format & MESA_ARRAY_FORMAT_BIT) > > - dst_mesa_format = > > _mesa_format_from_array_format(dst_format); > > - else > > - dst_mesa_format = dst_format; > > if (dst_internal_format != > > _mesa_get_format_base_format(dst_mesa_format)) { > > /* Compute src2rgba as > src->rgba->base->rgba */ > > uint8_t rgba2base[4], base2rgba[4], > swizzle[4]; > > @@ -580,11 +592,6 @@ _mesa_format_convert(void > *void_dst, > > uint32_t dst_format, size_t dst_stride, > > */ > > GLubyte rgba2base[4], base2rgba[4], > map[4]; > > bool need_convert = false; > > - mesa_format dst_mesa_format; > > - if (dst_format & MESA_ARRAY_FORMAT_BIT) > > - dst_mesa_format = > > _mesa_format_from_array_format(dst_format); > > - else > > - dst_mesa_format = dst_format; > > if (dst_internal_format != > > > _mesa_get_format_base_format(dst_mesa_format)) { > > > _mesa_compute_component_mapping(GL_RGBA, > > dst_internal_format, > > -- > > 1.9.1 > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev