[Mesa-dev] [PATCH v3 09/28] mesa: Add RGBA to Luminance conversion helpers
Iago Toral
itoral at igalia.com
Tue Dec 16 23:37:37 PST 2014
On Tue, 2014-12-16 at 10:54 -0800, Jason Ekstrand wrote:
>
>
> On Tue, Dec 16, 2014 at 4:01 AM, Iago Toral <itoral at 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
> <itoral at 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
> > > <itoral at 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?
I think it should work, although I wonder if this is better. This is how
I see this being implemented:
1) Do the 64-bit addition. This requires to allocate a temp buffer for
the 64-bit luminance values.
2) Clamp the 64-bit buffer to 32-bit values. Check if the dst format is
signed or not to decide if we need to do a signed or unsigned 32-bit
clamp. We don't have 64-bit to 32-bit integer clamp functions at the
moment, so those should be added.
3) If the dst type is 32-bit, then we copy that to dst.
3) If the dst type is <32 bits, copy to dst and clamp (considering
whether we need a signed or unsigned clamp again) => that means we need
to consider 4 paths here (unsigned byte, signed byte, unsigned short,
signed short)
In the end we have the same paths and the only gain is that we can do
without a macro to compute the additions but in exchange we need to
allocate a temporary buffer in all the scenarios and clamp twice for
types < 32-bit... so I wonder if this is really worth it in the end.
Maybe there is a better way to do it that I am missing?
> > >
> > > >
> > > > + }
> > > > + 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 at lists.freedesktop.org
> > > >
> > >
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
> > >
> > >
> >
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
More information about the mesa-dev
mailing list