[Mesa-dev] [PATCH 06/29] mesa: Avoid pack/unpack fast paths if base internal format != base format
Iago Toral
itoral at igalia.com
Thu Nov 20 23:03:58 PST 2014
On Thu, 2014-11-20 at 10:49 -0800, Jason Ekstrand wrote:
>
>
> On Wed, Nov 19, 2014 at 11:58 PM, Iago Toral <itoral at igalia.com>
> wrote:
> On Wed, 2014-11-19 at 11:57 -0800, Jason Ekstrand wrote:
> > A couple of general comments on this patch:
> >
> >
> > 1) The prerequisites should be moved to before the first
> patch in the
> > series and it should be squashed into the patch that
> introduces the
> > function. There are one or two more patches which also
> modify it and
> > those should also be squashed in.
>
> Ok.
>
> >
> > 2) I wonder if giving _mesa_format_convert an internal
> swizzle
> > wouldn't be better than a destination internal format.
> There are a
> > couple of reasons for this:
> >
> > a) It's more general. If it doesn't cost us anything
> extra to do
> > it that way, we might as well.
>
> I think that would only work directly for conversions between
> array
> formats, but what if we have, for example, a texture upload
> from RGB565
> to a Luminance format (so that we get an RGBA base format)?
> that would
> not go though _mesa_swizzle_and_convert and would require to
> unpack to
> RGBA and then pack to the dst... and unless the client has
> provided the
> dst format as an array format that won't use
> _mesa_swizzle_and_convert
> either. That should not be a problem when the calls to
> _mesa_format_convert come from things like glTexImage or
> glReadPixels,
> since in these cases the compute the dst format from a GL type
> and if it
> is an array format we should get that, but in other cases that
> might not
> be the case...
>
>
> I'm a bit confused about what you mean here. If the user passes in a
> non-trivial swizzle and we have to pack and unpack on both sides then
> we have to unpack, swizzle, and then repack. We would still have to
> do this if all you pass in is an internal format.
I was confused by the fact that we are currently not doing this in all
paths (I mean, swizzling after unpacking if necessary), but you are
right.
> Fortunately, the _mesa_swizzle_and_convert function can be used to do
> an in-place swizzle as long as the source and destination types have
> the same number of bits per pixel.
>
> If one side of the pack/repack is an array format, we can just build
> the swizzling into the one _mesa_swizzle_and_convert call.
Yes, I get your point now. Thanks!
>
> > b) It removes the GL enum dependency from the
> _mesa_format_convert
> > c) I think this implementation misses the case where we
> download a
> > texture that is storred in a different format than its base
> format.
> > For instance, if you are downloading a GL_RGB texture as
> GL_RGBA but
> > it's storred as GL_RGBA. I think with the current
> implementaion,
> > you'll get the junk in the alpha component of the texture's
> backing
> > storage instead of a constant alpha value of 1.
>
> That's correct. In the implementation of readpixels and
> getteximage we
> had to rebase the results in some cases to account for that.
>
> Iago
>
> >
> > On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
> > <itoral at igalia.com> wrote:
> > In general, if the dst base internal format and the
> selected
> > dst format are
> > different we can't be sure that directly packing or
> unpacking
> > to the destination
> > format will produce correct results. In these cases
> we
> > probably want to fall
> > back to other paths (like mesa_swizzle_and_convert)
> that can
> > handle these
> > situations better.
> >
> > An example of this includes a luminance internal
> format for
> > which the driver
> > decided to use something like BGRA. In these case,
> unpacking
> > to BGRA won't
> > match the requirements of the luminance internal
> format.
> >
> > In the future we may want to allow these fast paths
> for
> > specific cases
> > where we know the direct pack/unpack functions will
> do the
> > right thing.
> > ---
> > src/mesa/main/format_utils.c | 137
> > +++++++++++++++++++++++--------------------
> > 1 file changed, 72 insertions(+), 65 deletions(-)
> >
> > diff --git a/src/mesa/main/format_utils.c
> > b/src/mesa/main/format_utils.c
> > index 5964689..34c90d9 100644
> > --- a/src/mesa/main/format_utils.c
> > +++ b/src/mesa/main/format_utils.c
> > @@ -331,65 +331,82 @@ _mesa_format_convert(void
> *void_dst,
> > uint32_t dst_format, size_t dst_stride,
> > 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 &&
> > - !
> _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 &&
> > -
> _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;
> > + /* First we see if we can implement the
> conversion with a
> > direct pack
> > + * or unpack.
> > + *
> > + * In this case we want to be careful when the
> dst base
> > format and the
> > + * dst format do not match. In these cases a
> simple
> > pack/unpack to the
> > + * dst format from the src format may not match
> the
> > semantic requirements
> > + * of the internal base format. For now we
> decide to be
> > safe and avoid
> > + * this path in these scenarios but in the
> future we may
> > want to enable
> > + * it for specific combinations that are known
> to work.
> > + */
> > + 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;
> > + if
> (_mesa_get_format_base_format(dst_mesa_format) ==
> > dst_internal_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 &&
> > + !
>
> > _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 &&
> > +
> > _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;
> > }
> > - 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 &&
> > - !
> _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 &&
> > -
> _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;
> > + /* 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 &&
> > + !
>
> > _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 &&
> > +
> > _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;
> > }
> > - return;
> > }
> > }
> >
> > @@ -431,11 +448,6 @@ _mesa_format_convert(void
> *void_dst,
> > uint32_t dst_format, size_t dst_stride,
> > * 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;
> > 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];
> > @@ -580,11 +592,6 @@ _mesa_format_convert(void
> *void_dst,
> > uint32_t dst_format, size_t dst_stride,
> > */
> > 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;
> > if (dst_internal_format !=
> >
> _mesa_get_format_base_format(dst_mesa_format)) {
> >
> _mesa_compute_component_mapping(GL_RGBA,
> > dst_internal_format,
> > --
> > 1.9.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> >
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
>
>
>
>
>
More information about the mesa-dev
mailing list