<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Nov 19, 2014 at 11:42 PM, 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">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. <div><div class="h5"> > On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga > <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> 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. </div></div>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. </blockquote><div> </div><div>Right now we have 2 different swizzle formats floating around. Some with 6 components and some with 4. 4 works just fine as long as you are careful when you compose swizzles. With 6 you can avoid the if statements because 4 and 5 simply pass through. I like 4 better (which is why I did it that way in my stuff) but either works. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"> > > > + } > + 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 > <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>