<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>