On Wed, 2014-11-19 at 11:43 -0800, Jason Ekstrand wrote:
> A couple of specific comments are below. More generally, why are you
> only considering the base format on two cases? Do we never use it for
> anything else?
I thought about that too but when I looked at the original code it
seemed that it only cared for the base format in these two scenarios, so
I thought that maybe the conversions cases that could be affected are
all handled in those two paths. I'll check again though, just in case I
missed something.
> On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
> <ito...@igalia.com> wrote:
> Add a dst_internal_format parameter to _mesa_format_convert,
> that represents
> the base internal format for texture/pixel uploads, so we can
> do the right
> thing when the driver has selected a different internal format
> for the target
> dst format.
> ---
> src/mesa/main/format_utils.c | 65
> +++++++++++++++++++++++++++++++++++++++++++-
> src/mesa/main/format_utils.h | 2 +-
> 2 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/format_utils.c
> b/src/mesa/main/format_utils.c
> index fc59e86..5964689 100644
> --- a/src/mesa/main/format_utils.c
> +++ b/src/mesa/main/format_utils.c
> @@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum
> inFormat, GLenum outFormat, GLubyte *map)
> void
> _mesa_format_convert(void *void_dst, uint32_t dst_format,
> size_t dst_stride,
> void *void_src, uint32_t src_format,
> size_t src_stride,
> - size_t width, size_t height)
> + size_t width, size_t height, GLenum
> dst_internal_format)
> {
> uint8_t *dst = (uint8_t *)void_dst;
> uint8_t *src = (uint8_t *)void_src;
> @@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst,
> uint32_t dst_format, size_t dst_stride,
> if (src_array_format.as_uint && dst_array_format.as_uint)
> {
> assert(src_array_format.normalized ==
> dst_array_format.normalized);
>
> + /* If the base format of our dst is not the same as the
> provided base
> + * format it means that we are converting to a
> different format
> + * than the one originally requested by the client.
> This can happen when
> + * the internal base format requested is not supported
> by the driver.
> + * In this case we need to consider the requested
> internal base format to
> + * compute the correct swizzle operation from src to
> dst. We will do this
> + * 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;
>
>
> Let's put an extra line here so it doesn't get confused with the below
> if statement
>
>
> + 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];
> + _mesa_compute_component_mapping(GL_RGBA,
> dst_internal_format, rgba2base);
> + _mesa_compute_component_mapping(dst_internal_format,
> GL_RGBA, base2rgba);
> +
> + for (i = 0; i < 4; i++) {
> + if (base2rgba[i] > MESA_FORMAT_SWIZZLE_W)
> + swizzle[i] = base2rgba[i];
> + else
> + swizzle[i] =
> src2rgba[rgba2base[base2rgba[i]]];
>
>
> This doesn't work for composing three swizzles. If you get a ZERO or
> ONE in rgba2base, you'll read the wrong memory from src2rgba.
Actually, the problem is worse, because the mapping written by
_mesa_compute_component_mapping is a 6-component mapping and we are
passing a 4-component array. I'll fix this.
>
>
> + }
> + memcpy(src2rgba, swizzle, sizeof(src2rgba));
> + }
> +
> + /* Compute src2dst from src2rgba */
> for (i = 0; i < 4; i++) {
> if (rgba2dst[i] > MESA_FORMAT_SWIZZLE_W) {
> src2dst[i] = rgba2dst[i];
> @@ -539,9 +569,42 @@ _mesa_format_convert(void *void_dst,
> uint32_t dst_format, size_t dst_stride,
> src += src_stride;
> }
> } else {
> + /* For some conversions, doing src->rgba->dst is not
> enough and we
> + * need to consider the base internal format. In
> these cases a
> + * swizzle operation is required to match the
> semantics of the base
> + * internal format requested:
> src->rgba->swizzle->rgba->dst.
> + *
> + * We can detect these cases by checking if the
> swizzle transform
> + * for base->rgba->base is 0123. If it is not, then
> we need
> + * to do the swizzle operation (need_convert =
> true).
> + */
> + 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;
>
>
> Blank line again
>
>
>
> + if (dst_internal_format !=
> + _mesa_get_format_base_format(dst_mesa_format)) {
> + _mesa_compute_component_mapping(GL_RGBA,
> dst_internal_format,
> + base2rgba);
> +
> _mesa_compute_component_mapping(dst_internal_format, GL_RGBA,
> + rgba2base);
> + for (i = 0; i < 4; ++i) {
> + map[i] = base2rgba[rgba2base[i]];
> + if (map[i] != i)
> + need_convert = true;
> + }
> + }
> +
> for (row = 0; row < height; ++row) {
> _mesa_unpack_rgba_row(src_format, width,
> src, tmp_float + row *
> width);
> + if (need_convert)
> + _mesa_swizzle_and_convert(tmp_float + row *
> width, GL_FLOAT, 4,
> + tmp_float + row *
> width, GL_FLOAT, 4,
> + map, false, width);
> src += src_stride;
> }
> }
> diff --git a/src/mesa/main/format_utils.h
> b/src/mesa/main/format_utils.h
> index 4237ad3..29ab4a0 100644
> --- a/src/mesa/main/format_utils.h
> +++ b/src/mesa/main/format_utils.h
> @@ -158,6 +158,6 @@ _mesa_compute_component_mapping(GLenum
> inFormat, GLenum outFormat, GLubyte *map)
> void
> _mesa_format_convert(void *void_dst, uint32_t dst_format,
> size_t dst_stride,
> void *void_src, uint32_t src_format,
> size_t src_stride,
> - size_t width, size_t height);
> + size_t width, size_t height, GLenum
> dst_internal_format);
>
> #endif
> --
> 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
