On Thu, Nov 20, 2014 at 1:48 AM, Iago Toral <ito...@igalia.com> wrote:
> On Wed, 2014-11-19 at 11:28 -0800, Jason Ekstrand wrote: > > By and large, this looks good to me. Most of my comments are cosmetic > > or suggestions for added documentation. There is one issue that I > > think is subtly wrong with integer format conversion but that should > > be easy to fix. > > > > --Jason > > > > On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga > > <ito...@igalia.com> wrote: > > From: Jason Ekstrand <jason.ekstr...@intel.com> > > > > v2 by Iago Toral <ito...@igalia.com>: > > > > - When testing if we can directly pack we should use the src > > format to check > > if we are packing from an RGBA format. The original code > > used the dst format > > for the ubyte case by mistake. > > - Fixed incorrect number of bits for dst, it was computed > > using the src format > > instead of the dst format. > > - If the dst format is an array format, check if it is signed. > > We were only > > checking this for the case where it was not an array format, > > but we need > > to know this in both scenarios. > > - Fixed incorrect swizzle transform for the cases where we > > convert between > > array formats. > > - Compute is_signed and bits only once and for the dst format. > > We were > > computing these for the src format too but they were > > overwritten by the > > dst values immediately after. > > - Be more careful when selecting the integer path. > > Specifically, check that > > both src and dst are integer types. Checking only one of > > them should suffice > > since OpenGL does not allow conversions between normalized > > and integer types, > > but putting extra care here makes sense and also makes the > > actual requirements > > for this path more clear. > > - The format argument for pack functions is the destination > > format we are > > packing to, not the source format (which has to be RGBA). > > - Expose RGBA8888_* to other files. These will come in handy > > when in need to > > test if a given array format is RGBA or in need to pass RGBA > > formats to > > mesa_format_convert. > > > > v3 by Samuel Iglesias <sigles...@igalia.com>: > > > > - Add an RGBA8888_INT definition. > > --- > > src/mesa/main/format_utils.c | 378 > > +++++++++++++++++++++++++++++++++++++++++++ > > src/mesa/main/format_utils.h | 10 ++ > > src/mesa/main/formats.h | 15 +- > > 3 files changed, 399 insertions(+), 4 deletions(-) > > > > diff --git a/src/mesa/main/format_utils.c > > b/src/mesa/main/format_utils.c > > index fcbbba4..c3815cb 100644 > > --- a/src/mesa/main/format_utils.c > > +++ b/src/mesa/main/format_utils.c > > @@ -25,6 +25,384 @@ > > #include "format_utils.h" > > #include "glformats.h" > > #include "macros.h" > > +#include "format_pack.h" > > +#include "format_unpack.h" > > + > > +mesa_array_format RGBA8888_FLOAT = {{ > > + MESA_ARRAY_FORMAT_TYPE_FLOAT, > > + 0, > > + 4, > > + 0, 1, 2, 3, > > + 0, 1 > > +}}; > > + > > +mesa_array_format RGBA8888_UBYTE = {{ > > + MESA_ARRAY_FORMAT_TYPE_UBYTE, > > + 1, > > + 4, > > + 0, 1, 2, 3, > > + 0, 1 > > +}}; > > + > > +mesa_array_format RGBA8888_UINT = {{ > > + MESA_ARRAY_FORMAT_TYPE_UINT, > > + 0, > > + 4, > > + 0, 1, 2, 3, > > + 0, 1 > > +}}; > > + > > +mesa_array_format RGBA8888_INT = {{ > > + MESA_ARRAY_FORMAT_TYPE_INT, > > + 0, > > + 4, > > + 0, 1, 2, 3, > > + 0, 1 > > +}}; > > + > > +static void > > +invert_swizzle(uint8_t dst[4], const uint8_t src[4]) > > +{ > > + int i, j; > > + > > + dst[0] = MESA_FORMAT_SWIZZLE_NONE; > > + dst[1] = MESA_FORMAT_SWIZZLE_NONE; > > + dst[2] = MESA_FORMAT_SWIZZLE_NONE; > > + dst[3] = MESA_FORMAT_SWIZZLE_NONE; > > + > > + for (i = 0; i < 4; ++i) > > + for (j = 0; j < 4; ++j) > > + if (src[j] == i && dst[i] == > > MESA_FORMAT_SWIZZLE_NONE) > > + dst[i] = j; > > +} > > + > > +static GLenum > > +gl_type_for_array_format_datatype(enum > > mesa_array_format_datatype type) > > +{ > > + switch (type) { > > + case MESA_ARRAY_FORMAT_TYPE_UBYTE: > > + return GL_UNSIGNED_BYTE; > > + case MESA_ARRAY_FORMAT_TYPE_USHORT: > > + return GL_UNSIGNED_SHORT; > > + case MESA_ARRAY_FORMAT_TYPE_UINT: > > + return GL_UNSIGNED_INT; > > + case MESA_ARRAY_FORMAT_TYPE_BYTE: > > + return GL_BYTE; > > + case MESA_ARRAY_FORMAT_TYPE_SHORT: > > + return GL_SHORT; > > + case MESA_ARRAY_FORMAT_TYPE_INT: > > + return GL_INT; > > + case MESA_ARRAY_FORMAT_TYPE_HALF: > > + return GL_HALF_FLOAT; > > + case MESA_ARRAY_FORMAT_TYPE_FLOAT: > > + return GL_FLOAT; > > + default: > > + assert(!"Invalid datatype"); > > + return GL_NONE; > > + } > > +} > > > > > > We should probably just make _mesa_swizzle_and_convert take these > > instead of the GL types. That way we can completely remove GL from > > the format conversion code. I'm fine if that's a separate patch on > > top. > > Ok, I will try this. > > Just out of curiosity: is there any gain in avoiding the GL types in the > conversion code? > As I said in my reply to Jose on the 00/20 patch, we would like to eventually move the format conversion stuff to a common helper library that can be shared by mesa main and the gallium code. If we are going to do that, then we don't want any GL dependencies. > > > > > Also, it would be good to have a nice descriptive docstring on the > > function below. > > > > Oh, right! > > > > > + > > +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) > > +{ > > + uint8_t *dst = (uint8_t *)void_dst; > > + uint8_t *src = (uint8_t *)void_src; > > + mesa_array_format src_array_format, dst_array_format; > > + uint8_t src2dst[4], src2rgba[4], rgba2dst[4], dst2rgba[4]; > > + GLenum src_gl_type, dst_gl_type, common_gl_type; > > + bool normalized, dst_integer, src_integer, is_signed; > > + uint8_t (*tmp_ubyte)[4]; > > + float (*tmp_float)[4]; > > + uint32_t (*tmp_uint)[4]; > > + int i, bits; > > + size_t row; > > + > > + if (src_format & MESA_ARRAY_FORMAT_BIT) { > > + src_array_format.as_uint = src_format; > > + } else { > > + assert(_mesa_is_format_color_format(src_format)); > > + src_array_format.as_uint = > > _mesa_format_to_array_format(src_format); > > + } > > + > > + if (dst_format & MESA_ARRAY_FORMAT_BIT) { > > + dst_array_format.as_uint = dst_format; > > + } else { > > + assert(_mesa_is_format_color_format(dst_format)); > > + 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) { > > + assert(!_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) { > > + assert(_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; > > + } > > + } > > + > > + /* 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) { > > + assert(!_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) { > > + assert(_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; > > + } > > + } > > + > > + /* Handle conversions between array formats */ > > + normalized = false; > > + if (src_array_format.as_uint) { > > + src_gl_type = > > gl_type_for_array_format_datatype(src_array_format.type); > > + > > + src2rgba[0] = src_array_format.swizzle_x; > > + src2rgba[1] = src_array_format.swizzle_y; > > + src2rgba[2] = src_array_format.swizzle_z; > > + src2rgba[3] = src_array_format.swizzle_w; > > + > > + normalized = src_array_format.normalized; > > + } > > + > > + if (dst_array_format.as_uint) { > > + dst_gl_type = > > gl_type_for_array_format_datatype(dst_array_format.type); > > + > > + dst2rgba[0] = dst_array_format.swizzle_x; > > + dst2rgba[1] = dst_array_format.swizzle_y; > > + dst2rgba[2] = dst_array_format.swizzle_z; > > + dst2rgba[3] = dst_array_format.swizzle_w; > > + > > + invert_swizzle(rgba2dst, dst2rgba); > > + > > + normalized |= dst_array_format.normalized; > > + } > > + > > + if (src_array_format.as_uint && dst_array_format.as_uint) > > { > > + assert(src_array_format.normalized == > > dst_array_format.normalized); > > + > > + for (i = 0; i < 4; i++) { > > + if (rgba2dst[i] > MESA_FORMAT_SWIZZLE_W) { > > + src2dst[i] = rgba2dst[i]; > > + } else { > > + src2dst[i] = src2rgba[rgba2dst[i]]; > > + } > > + } > > + > > + for (row = 0; row < height; ++row) { > > + _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); > > + src += src_stride; > > + dst += dst_stride; > > + } > > + return; > > + } > > + > > + /* At this point, we're fresh out of fast-paths and we > > need to convert > > + * to float, uint32, or, if we're lucky, uint8. > > + */ > > + dst_integer = false; > > + src_integer = false; > > + > > + if (src_array_format.array_format_bit) { > > + if (!(src_array_format.type & > > MESA_ARRAY_FORMAT_TYPE_IS_FLOAT) && > > + !src_array_format.normalized) > > + src_integer = true; > > + } else { > > + switch (_mesa_get_format_datatype(src_format)) { > > + case GL_UNSIGNED_INT: > > + case GL_INT: > > + src_integer = true; > > + break; > > + } > > + } > > + > > + if (dst_array_format.array_format_bit) { > > + if (!(dst_array_format.type & > > MESA_ARRAY_FORMAT_TYPE_IS_FLOAT) && > > + !dst_array_format.normalized) > > + dst_integer = true; > > + is_signed = dst_array_format.as_uint & > > MESA_ARRAY_FORMAT_TYPE_IS_SIGNED; > > + bits = 8 * > > _mesa_array_format_datatype_size(dst_array_format.type); > > + } else { > > > > > > It's worth a quick comment here as to why we only need to look at the > > destination in order to determine the signedness of the intermediate > > storage. What it comes down to is this: If the destination format is > > signed but the source is unsigned, then we don't loose any data by > > converting to a signed format above and beyond the precision that we > > loose in the conversion itself. If the destination is unsigned then, > > by using an unsigned intermediate format, we make the conversion > > function that converts from the source to the intermediate format take > > care of truncating at zero. The exception here is if the intermediate > > format is float, in which case the first conversion will leave it > > signed and the second conversion will truncate at zero. > > Sure, I'll add a comment. > > > > > + switch (_mesa_get_format_datatype(dst_format)) { > > + case GL_UNSIGNED_NORMALIZED: > > + is_signed = false; > > + break; > > + case GL_SIGNED_NORMALIZED: > > + is_signed = true; > > + break; > > + case GL_FLOAT: > > + is_signed = true; > > + break; > > + case GL_UNSIGNED_INT: > > + is_signed = false; > > + dst_integer = true; > > + break; > > + case GL_INT: > > + is_signed = true; > > + dst_integer = true; > > + break; > > + } > > + bits = _mesa_get_format_max_bits(dst_format); > > + } > > + > > + assert((src_integer && dst_integer) || (!src_integer && ! > > dst_integer)); > > > > > > Wouldn't src_integer == dst_integer be a bit more clear here? > > Right, I guess I was a bit thick when I wrote that :) > > > > > + > > + if (src_integer && dst_integer) { > > + tmp_uint = malloc(width * height * sizeof(*tmp_uint)); > > > > > > There's another quick comment that would be good here. Namely that > > the [un]packing functions for unsigned datatypes treat the 32-bit > > integer array as signed for signed formats and as unsigned for > > unsigned formats. This is a bit of a problem if we ever convert from > > a signed to an unsigned format because the unsigned packing function > > doesn't know that the input is signed and will treat it as unsigned > > and not do the trunctation. The thing that saves us here is that all > > of the packed formats are unsigned, so we can just always use > > _mesa_swizzle_and_convert for signed formats and it is aware of the > > truncation problem. This may be worth an assertion somewhere. > > Sure, that's helpful. > > > > > > > + common_gl_type = is_signed ? GL_INT : GL_UNSIGNED_INT; > > + > > + if (src_format & MESA_ARRAY_FORMAT_BIT) { > > > > > > In light of the above coment, I think we want to use "if > > (src_array_format)" to ensure that we use _mesa_swizzle_and_convert > > for all signed source formats > > I will try this. > > > > > + for (row = 0; row < height; ++row) { > > + _mesa_swizzle_and_convert(tmp_uint + row * width, > > common_gl_type, 4, > > + src, src_gl_type, > > + > > src_array_format.num_channels, > > + src2rgba, normalized, > > width); > > + src += src_stride; > > + } > > + } else { > > + for (row = 0; row < height; ++row) { > > + _mesa_unpack_uint_rgba_row(src_format, width, > > + src, tmp_uint + row * > > width); > > + src += src_stride; > > + } > > + } > > + > > + if (dst_format & MESA_ARRAY_FORMAT_BIT) { > > > > > > This one's fine because, at this point, we have already done the > > truncation if the source is signed but the destination is unsigned. > > I'll add a commment to clarify this too. > > > > > + for (row = 0; row < height; ++row) { > > + _mesa_swizzle_and_convert(dst, dst_gl_type, > > + > > dst_array_format.num_channels, > > + tmp_uint + row * width, > > common_gl_type, 4, > > + rgba2dst, normalized, > > width); > > + dst += dst_stride; > > + } > > + } else { > > + for (row = 0; row < height; ++row) { > > + _mesa_pack_uint_rgba_row(dst_format, width, > > + (const uint32_t > > (*)[4])tmp_uint + row * width, dst); > > + dst += dst_stride; > > + } > > + } > > + > > + free(tmp_uint); > > + } else if (is_signed || bits > 8) { > > + tmp_float = malloc(width * height * > > sizeof(*tmp_float)); > > + > > + if (src_format & MESA_ARRAY_FORMAT_BIT) { > > + for (row = 0; row < height; ++row) { > > + _mesa_swizzle_and_convert(tmp_float + row * > > width, GL_FLOAT, 4, > > + src, src_gl_type, > > + > > src_array_format.num_channels, > > + src2rgba, normalized, > > width); > > + src += src_stride; > > + } > > + } else { > > + for (row = 0; row < height; ++row) { > > + _mesa_unpack_rgba_row(src_format, width, > > + src, tmp_float + row * > > width); > > + src += src_stride; > > + } > > + } > > + > > + if (dst_format & MESA_ARRAY_FORMAT_BIT) { > > + for (row = 0; row < height; ++row) { > > + _mesa_swizzle_and_convert(dst, dst_gl_type, > > + > > dst_array_format.num_channels, > > + tmp_float + row * > > width, GL_FLOAT, 4, > > + rgba2dst, normalized, > > width); > > + dst += dst_stride; > > + } > > + } else { > > + for (row = 0; row < height; ++row) { > > + _mesa_pack_float_rgba_row(dst_format, width, > > + (const float > > (*)[4])tmp_float + row * width, dst); > > + dst += dst_stride; > > + } > > + } > > + > > + free(tmp_float); > > + } else { > > + tmp_ubyte = malloc(width * height * > > sizeof(*tmp_ubyte)); > > + > > + if (src_format & MESA_ARRAY_FORMAT_BIT) { > > + for (row = 0; row < height; ++row) { > > + _mesa_swizzle_and_convert(tmp_ubyte + row * > > width, GL_UNSIGNED_BYTE, 4, > > + src, src_gl_type, > > + > > src_array_format.num_channels, > > + src2rgba, normalized, > > width); > > + src += src_stride; > > + } > > + } else { > > + for (row = 0; row < height; ++row) { > > + _mesa_unpack_ubyte_rgba_row(src_format, width, > > + src, tmp_ubyte + row > > * width); > > + src += src_stride; > > + } > > + } > > + > > + if (dst_format & MESA_ARRAY_FORMAT_BIT) { > > + for (row = 0; row < height; ++row) { > > + _mesa_swizzle_and_convert(dst, dst_gl_type, > > + > > dst_array_format.num_channels, > > + tmp_ubyte + row * > > width, GL_UNSIGNED_BYTE, 4, > > + rgba2dst, normalized, > > width); > > + dst += dst_stride; > > + } > > + } else { > > + for (row = 0; row < height; ++row) { > > + _mesa_pack_ubyte_rgba_row(dst_format, width, > > + (const uint8_t > > (*)[4])tmp_ubyte + row * width, dst); > > + dst += dst_stride; > > + } > > + } > > + > > + free(tmp_ubyte); > > + } > > +} > > > > static const uint8_t map_identity[7] = { 0, 1, 2, 3, 4, 5, > > 6 }; > > static const uint8_t map_3210[7] = { 3, 2, 1, 0, 4, 5, 6 }; > > diff --git a/src/mesa/main/format_utils.h > > b/src/mesa/main/format_utils.h > > index 010d137..4d348cc 100644 > > --- a/src/mesa/main/format_utils.h > > +++ b/src/mesa/main/format_utils.h > > @@ -33,6 +33,11 @@ > > > > #include "imports.h" > > > > +extern mesa_array_format RGBA8888_FLOAT; > > +extern mesa_array_format RGBA8888_UBYTE; > > +extern mesa_array_format RGBA8888_UINT; > > +extern mesa_array_format RGBA8888_INT; > > + > > /* Only guaranteed to work for BITS <= 32 */ > > #define MAX_UINT(BITS) ((BITS) == 32 ? UINT32_MAX : ((1u << > > (BITS)) - 1)) > > #define MAX_INT(BITS) ((int)MAX_UINT((BITS) - 1)) > > @@ -147,4 +152,9 @@ _mesa_swizzle_and_convert(void *dst, > > GLenum dst_type, int num_dst_channels, > > const void *src, GLenum src_type, > > int num_src_channels, > > const uint8_t swizzle[4], bool > > normalized, int count); > > > > +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); > > + > > #endif > > diff --git a/src/mesa/main/formats.h b/src/mesa/main/formats.h > > index bba5bae..7a792d4 100644 > > --- a/src/mesa/main/formats.h > > +++ b/src/mesa/main/formats.h > > @@ -81,7 +81,7 @@ enum { > > MESA_FORMAT_SWIZZLE_NONE = 6, > > }; > > > > -enum mesa_array_format_datatype { > > +enum mesa_array_format_datatype { > > MESA_ARRAY_FORMAT_TYPE_UBYTE = 0x0, > > MESA_ARRAY_FORMAT_TYPE_USHORT = 0x1, > > MESA_ARRAY_FORMAT_TYPE_UINT = 0x2, > > @@ -90,11 +90,12 @@ enum mesa_array_format_datatype { > > MESA_ARRAY_FORMAT_TYPE_INT = 0x6, > > MESA_ARRAY_FORMAT_TYPE_HALF = 0xd, > > MESA_ARRAY_FORMAT_TYPE_FLOAT = 0xe, > > - > > - MESA_ARRAY_FORMAT_TYPE_IS_SIGNED = 0x4, > > - MESA_ARRAY_FORMAT_TYPE_IS_FLOAT = 0x8, > > }; > > > > +#define MESA_ARRAY_FORMAT_TYPE_IS_SIGNED 0x4 > > +#define MESA_ARRAY_FORMAT_TYPE_IS_FLOAT 0x8 > > +#define MESA_ARRAY_FORMAT_BIT 0x80000000 > > + > > typedef union { > > struct { > > enum mesa_array_format_datatype type:4; > > @@ -130,6 +131,12 @@ _mesa_ilog2(unsigned x) > > (((SIGNED) << 2 ) & MESA_ARRAY_FORMAT_TYPE_IS_SIGNED) | > > \ > > (((IS_FLOAT) << 3 ) & MESA_ARRAY_FORMAT_TYPE_IS_FLOAT) > > > > +static inline int > > +_mesa_array_format_datatype_size(enum > > mesa_array_format_datatype type) > > +{ > > + return 1 << (type & 0x3); > > +} > > + > > /** > > * Mesa texture/renderbuffer image formats. > > */ > > > > > > Let's squash these changes into the pach where we first introduce > > array formats. > > Ok. > > > > > -- > > 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