[Mesa-dev] [Bug 84566] Unify the format conversion code

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=84566

--- Comment #52 from Samuel Iglesias <siglesias at igalia.com> ---
(In reply to Jason Ekstrand from comment #51)
> (In reply to Samuel Iglesias from comment #48)
> > (In reply to Jason Ekstrand from comment #34)
> > > (In reply to Samuel Iglesias from comment #33)
> > > > Jason, I would like to know your opinion about the integer RGBA clamping
> > > > done in pack.c (_mesa_pack_rgba_span_from_ints()).
> > > > 
> > > > glReadPixels() and glGetTexImage() specs said (for example, in OpenGL 3.3.
> > > > core specification) that for an integer RGBA color, each component is
> > > > clamped to the representable range of type.
> > > > 
> > > > Those GL functions end up calling pack.c's functions for performing the
> > > > conversion (mesa_pack_rgba_span_from_ints() and others).
> > > > 
> > > > It's possible to replace some of those pack.c's conversion functions by the
> > > > master conversion but the problem is in the clamping operation. I would like
> > > > to add a boolean argument called "clamp" to the master conversion function
> > > > which is passed to _mesa_swizzle_and_convert() where each of its
> > > > convert_{uint,int,byte,ubyte,short,ushort}() do the right thing when "clamp"
> > > > is true.
> > > > 
> > > > This "clamp" parameter would be false for every call to either master
> > > > conversion function or _mesa_swizzle_and_convert() except when they are
> > > > being called from pack.c.
> > > > 
> > > > What do you think?
> > > 
> > > Supporting clamping is probably fine.  I think we determined we needed a
> > > clamp parameter anyway for some of the float conversions.  I guess it makes
> > > sense for integers too.  Let's watch out for performance when we implement
> > > it though.  Changing the loop inside mesa_swizzle_and_convert without
> > > hurting performance can be tricky.  The teximage-colors test in Piglit has a
> > > -benchmark flag that can be used for testing that.
> > 
> > In the end, we did not make that change in pack.c as we could just use the
> > autogenerated format pack/unpack functions. However I am retaking this topic
> > again because we found another issue which would require a similar solution:
> > 
> > The convert_*() functions in format_utils.c convert between source and
> > destination data and are used by _mesa_swizzle_and_convert. We found that
> > these were not good enough for various conversions that involved
> > non-normalized types of different sizes: INT to SHORT, INT to BYTE, etc.
> > Because of that, several piglit tests related to glGetTexImage() and others
> > failed, like for example bin/ext_texture_integer-getteximage-clamping.
> > 
> > In order to fix that we added the clamp expressions for these cases [1]  and
> > with that we achieved no regressions when executing a full piglit run on
> > i965 driver.
> > Unfortunately, when testing our patches on a couple of Gallium drivers, we
> > found a regression that we tracked down back to that patch:
> > bin/arb_clear_buffer_object-formats.
> > Reverting the patch makes fixes the problem with these Gallium drivers but
> > then, bin/ext_texture_integer-getteximage-clamping fails again on i965. We
> > wonder if there could be more cases like this that piglit is not covering,
> > since it looks weird that this affects just this one test.
> > 
> > So, we wonder if that patch for _mesa_swizzle_and_convert is correct and we
> > should fix the failed gallium cases elsewhere, or if we should revert that
> > patch and fix the cases it fixed in a different way. What do you think? Was
> > _mesa_swizzle_and_convert implemented like that  on purpose or are these
> > real bugs?
> 
> From my brief reading of the GL spec, it looks like clamping integers to the
> max representable range is what it expects by default.  From the glTexImage
> spec:
> 
> "The selected groups are transferred to the GL as described in section 3.7.2
> and then clamped to the representable range of the internal format. If the
> internalformat of the texture is signed or unsigned integer, components are
> clamped to [-2^(n-1), 2^(n-1)-1] or [0, 2^(n-1)-1], respectively, where n is
> the number of bits per component. For color component groups, if the
> internalformat of the texture is signed or unsigned normalized fixed-point,
> components are clamped t0 [-1, 1] or [0, 1], respectively."
> 
> Therefore, it seems as if we want to be clamping when we have integer
> destinations.  I'm not sure why the gallium drivers are regressing when you
> do.
> 
> One more observation is that it doesn't look like your clamping patch is
> complete.  If we're going to clamp when we have an integer destination, we
> should always clamp with integer destinations, not just in the few cases
> that piglit hits.  I wouldn't be surprised if piglit's coverage in those
> areas is terrible.
>

Yes, you are right about the specification. We have added clamping for all the
needed conversions in _mesa_swizzle_and_convert() following the spec but, after
analyzing why gallium drivers have this regression, I discovered that it comes
from other place.

The following text was copied from piglit's arb_clear_buffer_object-formats
program source code explaining its purpose:

   Test clearing the entire buffer with multiple internal formats. Pass the
data
   to clear the buffer with in a format so that the GL doesn't have to do any
   format conversion.

When "bin/arb_clear_buffer_object-formats" program was testing
glClearBufferData() with GL_LUMINANCE_ALPHA16I_EXT and
GL_LUMINANCE_ALPHA32I_EXT internal formats, they were not assigned to their
respective MESA_FORMAT_LA_SINT16 and MESA_FORMAT_LA_SINT32 mesa formats but to
MESA_FORMAT_LA_SINT8 and MESA_FORMAT_LA_SINT16 in get_texbuffer_format().

Then memcpy path was not executed, texstore_rgba() was called instead and, as
we added clamping in _mesa_swizzle_and_convert(), the bug appeared. I wrote a
patch fixing that function and now everything is working as expected.

This program don't test these internal formats in i965 because they are not
part of the core profile. So we had this bug there without noticing.

Thanks for your comments.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141107/1e665dda/attachment.html>