On Wed, Nov 19, 2014 at 11:42 PM, Iago Toral <ito...@igalia.com> wrote:
> 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.
>
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.
>
> >
> >
> > + }
> > + 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
