[Mesa-dev] [PATCH 06/29] mesa: Avoid pack/unpack fast paths if base internal format != base format
Jason Ekstrand
jason at jlekstrand.net
Thu Nov 20 10:49:52 PST 2014
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. 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.
> 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
> >
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141120/0c55b165/attachment-0001.html>
More information about the mesa-dev
mailing list