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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #48 from Samuel Iglesias <siglesias at igalia.com> ---
(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?

If we decide to revert our clamp patch, then a solution could be to have a
separate implementation of _mesa_swizzle_and_convert() and its convert_*()
functions that clamps. We would have to use that version in glGetTexImage()
(maybe in glReadPIxels too)  and use the normal version for texture uploads
(texstore). If we do this, then we would have to add a clamp flag to
_mesa_format_convert, the same flag to _mesa_swizzle_and_convert and the have a
new collection of convert_*_clamp() functions for the sake of performance (so
we don't have to check whether we have to clamp or not for each pixel we
process).  We have tested this already and it works fine (no regressions) for
i965. We also repeated the full piglit run on Gallium 0.4 on AMD RV710 and it
showed zero regressions. And the benchmark you mention in the previous comment
showed that there weren't significative differences in the total time of both
runs (with and without the patch).

What do you think? Should we keep the clamp patch? Would you suggest a
different approach?

[1] http://pastebin.com/CPYbCU8e

-- 
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/20141104/ebcd71e3/attachment.html>