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...
> 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
