On Tue, 2014-12-16 at 08:29 +0100, Iago Toral wrote:
> On Mon, 2014-12-15 at 10:19 -0800, Jason Ekstrand wrote:
> >
> >
> > On Mon, Dec 15, 2014 at 3:12 AM, Iago Toral <ito...@igalia.com> wrote:
> > On Fri, 2014-12-12 at 11:36 -0800, Jason Ekstrand wrote:
> > >
> > >
> > > On Tue, Dec 9, 2014 at 4:07 AM, Iago Toral Quiroga
> > <ito...@igalia.com>
> > > wrote:
> > > For glReadPixels with a Luminance destination format
> > we
> > > compute luminance
> > > values from RGBA as L=R+G+B. This, however, requires
> > ad-hoc
> > > implementation,
> > > since pack/unpack functions or
> > _mesa_swizzle_and_convert won't
> > > do this
> > > (and thus, neither will _mesa_format_convert). This
> > patch adds
> > > helpers
> > > to do this computation so they can be used to
> > support
> > > conversion to luminance
> > > formats.
> > >
> > > The current implementation of glReadPixels does this
> > > computation as part
> > > of the span functions in pack.c (see
> > > _mesa_pack_rgba_span_float), that do
> > > this together with other things like type
> > conversion, etc. We
> > > do not want
> > > to use these functions but use _mesa_format_convert
> > instead
> > > (later patches
> > > will remove the color span functions), so we need to
> > extract
> > > this functionality
> > > as helpers.
> > > ---
> > > src/mesa/main/pack.c | 63
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > src/mesa/main/pack.h | 9 ++++++++
> > > 2 files changed, 72 insertions(+)
> > >
> > > diff --git a/src/mesa/main/pack.c
> > b/src/mesa/main/pack.c
> > > index de6ab27..fa4046c 100644
> > > --- a/src/mesa/main/pack.c
> > > +++ b/src/mesa/main/pack.c
> > > @@ -4334,4 +4334,67 @@ _mesa_rebase_rgba_uint(GLuint
> > n, GLuint
> > > rgba[][4], GLenum baseFormat)
> > > }
> > > }
> > >
> > > +void
> > > +_mesa_pack_luminance_from_rgba_float(GLuint n,
> > GLfloat
> > > rgba[][4],
> > > + GLvoid
> > *dstAddr, GLenum
> > > dst_format,
> > > + GLbitfield
> > transferOps)
> > > +{
> > > + int i;
> > > + GLfloat *dst = (GLfloat *) dstAddr;
> > > +
> > > + switch (dst_format) {
> > > + case GL_LUMINANCE:
> > > + if (transferOps & IMAGE_CLAMP_BIT) {
> > > + for (i = 0; i < n; i++) {
> > > + GLfloat sum = rgba[i][RCOMP] +
> > rgba[i][GCOMP] +
> > > rgba[i][BCOMP];
> > > + dst[i] = CLAMP(sum, 0.0F, 1.0F);
> > > + }
> > > + } else {
> > > + for (i = 0; i < n; i++) {
> > > + dst[i] = rgba[i][RCOMP] +
> > rgba[i][GCOMP] +
> > > rgba[i][BCOMP];
> > > + }
> > > + }
> > > + return;
> > > + case GL_LUMINANCE_ALPHA:
> > > + if (transferOps & IMAGE_CLAMP_BIT) {
> > > + for (i = 0; i < n; i++) {
> > > + GLfloat sum = rgba[i][RCOMP] +
> > rgba[i][GCOMP] +
> > > rgba[i][BCOMP];
> > > + dst[2*i] = CLAMP(sum, 0.0F, 1.0F);
> > > + dst[2*i+1] = rgba[i][ACOMP];
> > > + }
> > > + } else {
> > > + for (i = 0; i < n; i++) {
> > > + dst[2*i] = rgba[i][RCOMP] +
> > rgba[i][GCOMP] +
> > > rgba[i][BCOMP];
> > > + dst[2*i+1] = rgba[i][ACOMP];
> > > + }
> > > + }
> > > + return;
> > > + default:
> > > + assert(!"Unsupported format");
> > > + }
> > > +}
> > > +
> > > +void
> > > +_mesa_pack_luminance_from_rgba_integer(GLuint n,
> > GLuint
> > > rgba[][4],
> > > + GLvoid
> > *dstAddr,
> > > GLenum dst_format)
> > > +{
> > > + int i;
> > > + GLuint *dst = (GLuint *) dstAddr;
> > > +
> > > + switch (dst_format) {
> > > + case GL_LUMINANCE:
> > > + for (i = 0; i < n; i++) {
> > > + dst[i] = rgba[i][RCOMP] + rgba[i][GCOMP] +
> > > rgba[i][BCOMP];
> > >
> > >
> > > I know this is getting old, but don't we need to do some
> > clamping
> > > here? If we do, then we have to handle signed vs. unsigned
> > > seperately. Yeah, that's annoying.
> >
> >
> > You are right, reviewing the original code it looks like it
> > ends up
> > clamping the result of the triple addition to the type of the
> > dst when
> > the type of the dst has a different size.
> >
> > In our case, we call this function from the implementation of
> > glreadpixels when an integer dst luminance format is involved.
> > In that
> > case we convert to RGBA_INT RGBA_UINT first depending on the
> > type of the
> > luminance format and then we call this function, so I think we
> > only need
> > to handle clamping when we pack from RGBA_INT to short and
> > when we pack
> > from RGBA_UINT to unsigned short (meaning that we do not need
> > to handle
> > scenarios where we convert from int to ushort for example).
> > I'll fix
> > this.
> >
> >
> > Yeah, RGBA_[U]INT in is the only case that matters there. What were
> > you thinking here? Personally, I'd probably just make this function
> > clamp and then not special case it. That said, I'm up for other
> > solutions too.
>
> My plan was:
>
> 1) Add a "GLenum type" parameter so we can detect if we are in the int
> case (GL_BYTE, GL_SHORT, etc) or the uint case (GL_UNSIGNED_BYTE,
> GL_UNSIGNED_SHORT, etc).
>
> 2) If we are clamping uints we use _mesa_unsigned_to_unsigned to clamp.
> If we are clamping ints, we use _mesa_signed_to_signed. This is because
> glReadPixels converts from the render buffer format to RGBA UINT when
> the dst type is uint and to RGBA INT otherwise before calling this
> function.
>
> It would look something like this (pseudocode):
>
> void
> _mesa_pack_luminance_from_rgba_integer(GLuint n, GLuint rgba[][4],
> GLvoid *dstAddr, GLenum dst_format, GLenum dst_type)
> {
> // Compute L = R + G + B as we do now, without clamping
>
> // Handle clamping if necessary (only if the dst_type has
> // a different size
> int bits = bits = _mesa_sizeof_type(dst_type) * 8;
> if (bits < 32) {
> int components = _mesa_components_in_format(dst_format);
> int is_signed = !_mesa_is_type_unsigned(dst_type);
> if (is_signed)
> for (i = 0; i < n; i++) {
> dst[i*components] =
> _mesa_signed_to_signed(dst[i*components], bits);
> } else {
> for (i = 0; i < n; i++) {
> dst[i*components] =
> _mesa_unsigned_to_unsigned(dst[i*components], bits);
> }
> }
> }
>
> We could add the unsigned_to_signed and signed_to_unsigned paths too if
> you prefer it that way, but they won't be used as far as I can see. In
> that case we would also need to add a src_type parameter.
Well, in the end I could not do it like this. There is a subtle problem
with the RB format -> RGBA conversion that we do before we pack the
luminance values. The problem is that we do not want this conversion to
ever do int->uint clamping because we want to compute L=R+G+B with
unclamped values first, then clamp the result.
For example, if we have a signed integer render buffer and a pixel in
that buffer has this value:
r=-5, g=6, b=1
when we convert this to RGBA we want to have
r=-5, g=6, b=1, a = 1
instead of
r=0, g=6, b=1, a = 1
which is what we would get if we convert to RGBA UINT. Then we would
compute L = R+G+B = 2, and this is the value we should clamp to the type
of the dst if we need to.
So in the end I have to consider signed<->unsigned conversions when we
compute L anyway.
Here is a pastebin with the actual implementation of the luminance
packing helper:
http://pastebin.com/gB0aLtzx
I tested this with a few examples and seems to produce correct results.
Notice that this is currently broken in master, because pack.c defines
stuff like this:
#define SRC_CONVERT(x) CLAMP((int)x, -32768, 32767)
#define FN_NAME pack_short_from_uint_rgba
#define SRC_CONVERT(x) CLAMP((int)x, -128, 127)
#define FN_NAME pack_byte_from_uint_rgba
which does not do the right thing when 'x' is expanded to be an
expression that adds R, G and B components.
Iago
> >
> >
> > Iago
> >
> > >
> > > + }
> > > + return;
> > > + case GL_LUMINANCE_ALPHA:
> > > + for (i = 0; i < n; i++) {
> > > + dst[2*i] = rgba[i][RCOMP] + rgba[i][GCOMP]
> > +
> > > rgba[i][BCOMP];
> > > + dst[2*i+1] = rgba[i][ACOMP];
> > > + }
> > > + return;
> > > + default:
> > > + assert(!"Unsupported format");
> > > + }
> > > +}
> > >
> > > diff --git a/src/mesa/main/pack.h
> > b/src/mesa/main/pack.h
> > > index 2173b65..2783f23 100644
> > > --- a/src/mesa/main/pack.h
> > > +++ b/src/mesa/main/pack.h
> > > @@ -155,4 +155,13 @@ _mesa_rebase_rgba_float(GLuint
> > n, GLfloat
> > > rgba[][4], GLenum baseFormat);
> > > extern void
> > > _mesa_rebase_rgba_uint(GLuint n, GLuint rgba[][4],
> > GLenum
> > > baseFormat);
> > >
> > > +extern void
> > > +_mesa_pack_luminance_from_rgba_float(GLuint n,
> > GLfloat
> > > rgba[][4],
> > > + GLvoid
> > *dstAddr, GLenum
> > > dst_format,
> > > + GLbitfield
> > transferOps);
> > > +
> > > +extern void
> > > +_mesa_pack_luminance_from_rgba_integer(GLuint n,
> > GLuint
> > > rgba[][4],
> > > + GLvoid
> > *dstAddr,
> > > GLenum dst_format);
> > > +
> > > #endif
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > >
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
