On Tue, Dec 16, 2014 at 4:01 AM, Iago Toral <ito...@igalia.com> wrote:
>
> 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.
>
Yes, I think that works but is a bit more complicated than needed. Why
can't we just do a 64-bit add of the components and then clamp to a 32-bit
value. If the final value is 8 or 16 bits, it will get clamped again, but
that shouldn't be a problem. We will have to differentiate between signed
and unsigned, but that's ok. It would be much simpler. Would that work or
am I missing something?
--Jason
>
> 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
