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?
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.
> + }
> + 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
