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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #53 from Jason Ekstrand <jason at jlekstrand.net> ---
(In reply to Samuel Iglesias from comment #52)
> (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.

Good work!

-- 
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/8813a96f/attachment-0001.html>