<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Dec 16, 2014 at 4:01 AM, 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 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. </div></div>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. </blockquote><div> </div><div>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>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Iago <div class="HOEnZb"><div class="h5"> > > > > > > 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 > > > <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>