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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #51 from Jason Ekstrand <jason at jlekstrand.net> ---
(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.

> 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).

At this point, it seems like a little more digging into what's happening on
your RV710 would be good.

-- 
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/20141106/62619726/attachment.html>