https://bugs.freedesktop.org/show_bug.cgi?id=84566
--- Comment #53 from Jason Ekstrand <ja...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
