<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Nov 20, 2014 at 1:48 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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 > <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote: > From: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> > > v2 by Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>>: > > - 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 <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>>: > > - 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. </div></div>Ok, I will try this. Just out of curiosity: is there any gain in avoiding the GL types in the conversion code? </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > Also, it would be good to have a nice descriptive docstring on the > function below. > Oh, right! <div><div class="h5"> > > + > +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. </div></div>Sure, I'll add a comment. <div><div class="h5"> > > + 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? </div></div>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. <div><div class="h5"> > > + 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. </div></div>Ok. <div class="HOEnZb"><div class="h5"> > > -- > 1.9.1 > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>