<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Dec 16, 2014 at 11:37 PM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, 2014-12-16 at 10:54 -0800, Jason Ekstrand wrote: > > > On Tue, Dec 16, 2014 at 4:01 AM, Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> 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 > <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> 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 > > > <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> > > > > 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: > <a href="http://pastebin.com/gB0aLtzx" target="_blank">http://pastebin.com/gB0aLtzx</a> > > 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? </div></div>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? </blockquote><div> </div><div>Oh, I wasn't thinking a 64bit temporary at all. Just cast to the appropreate 64-bit integer type, add, clamp and go back to 32 in one smooth motion in the original clampping function you had. All of the malloc'd storage stays 32-bit, it just tweaks the math a bit. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"> > > > > > > > > > > > + } > > > > + 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 > > > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > > > > > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > > > > > > > > > > > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > </div></div></blockquote></div></div></div>