Patch looks good. I don't have the CTS test accessible, so I couldn't
confirm myself that it is fixed. If that's confirmed, patch is:
Reviewed-by: Eduardo Lima Mitev <el...@igalia.com>
Left a few long-line nitpicks below.
Eduardo
On 05/10/2016 05:59 AM, Dave Airlie wrote:
> From: Dave Airlie <airl...@redhat.com>
>
> The clamping code always clamps to 0..1, which for SNORM is
> incorrect. This adds a bit to say that clamping is for
> an snorm and to use the correct range.
>
> This fixes a number of SNORM cases in:
> GL33-CTS.texture_size_promotion.functional
>
> Signed-off-by: Dave Airlie <airl...@redhat.com>
> ---
> src/mesa/main/pack.c | 4 +++-
> src/mesa/main/pixeltransfer.c | 10 ++++++----
> src/mesa/main/pixeltransfer.h | 1 +
> src/mesa/main/readpix.c | 8 +++++++-
> src/mesa/main/texgetimage.c | 4 +++-
> src/mesa/main/texstore.c | 4 +++-
> src/mesa/swrast/s_copypix.c | 4 +++-
> src/mesa/swrast/s_drawpix.c | 2 +-
> 8 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
> index 89faf51..f810929 100644
> --- a/src/mesa/main/pack.c
> +++ b/src/mesa/main/pack.c
> @@ -1607,7 +1607,9 @@ _mesa_unpack_color_index_to_rgba_float(struct
> gl_context *ctx, GLuint dims,
> * with color indexes.
> */
> transferOps &= ~(IMAGE_SCALE_BIAS_BIT | IMAGE_MAP_COLOR_BIT);
> - _mesa_apply_rgba_transfer_ops(ctx, transferOps, count, (float
> (*)[4])dstPtr);
> + _mesa_apply_rgba_transfer_ops(ctx, transferOps,
> + false,
> + count, (float (*)[4])dstPtr);
>
> dstPtr += srcHeight * srcWidth * 4;
> }
> diff --git a/src/mesa/main/pixeltransfer.c b/src/mesa/main/pixeltransfer.c
> index 22eac00..93ab8ad 100644
> --- a/src/mesa/main/pixeltransfer.c
> +++ b/src/mesa/main/pixeltransfer.c
> @@ -162,6 +162,7 @@ _mesa_scale_and_bias_depth_uint(const struct gl_context
> *ctx, GLuint n,
> */
> void
> _mesa_apply_rgba_transfer_ops(struct gl_context *ctx, GLbitfield transferOps,
> + bool clamp_snorm,
> GLuint n, GLfloat rgba[][4])
> {
> /* scale & bias */
> @@ -180,11 +181,12 @@ _mesa_apply_rgba_transfer_ops(struct gl_context *ctx,
> GLbitfield transferOps,
> /* clamping to [0,1] */
> if (transferOps & IMAGE_CLAMP_BIT) {
> GLuint i;
> + GLfloat lower_val = clamp_snorm ? -1.0F : 0.0F;
> for (i = 0; i < n; i++) {
> - rgba[i][RCOMP] = CLAMP(rgba[i][RCOMP], 0.0F, 1.0F);
> - rgba[i][GCOMP] = CLAMP(rgba[i][GCOMP], 0.0F, 1.0F);
> - rgba[i][BCOMP] = CLAMP(rgba[i][BCOMP], 0.0F, 1.0F);
> - rgba[i][ACOMP] = CLAMP(rgba[i][ACOMP], 0.0F, 1.0F);
> + rgba[i][RCOMP] = CLAMP(rgba[i][RCOMP], lower_val, 1.0F);
> + rgba[i][GCOMP] = CLAMP(rgba[i][GCOMP], lower_val, 1.0F);
> + rgba[i][BCOMP] = CLAMP(rgba[i][BCOMP], lower_val, 1.0F);
> + rgba[i][ACOMP] = CLAMP(rgba[i][ACOMP], lower_val, 1.0F);
> }
> }
> }
> diff --git a/src/mesa/main/pixeltransfer.h b/src/mesa/main/pixeltransfer.h
> index b0a301f..e67ab53 100644
> --- a/src/mesa/main/pixeltransfer.h
> +++ b/src/mesa/main/pixeltransfer.h
> @@ -56,6 +56,7 @@ _mesa_scale_and_bias_depth_uint(const struct gl_context
> *ctx, GLuint n,
>
> extern void
> _mesa_apply_rgba_transfer_ops(struct gl_context *ctx, GLbitfield transferOps,
> + bool clamp_snorm,
> GLuint n, GLfloat rgba[][4]);
>
> extern void
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index 1cb06c7..ebdcf96 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -119,6 +119,10 @@ _mesa_get_readpixels_transfer_ops(const struct
> gl_context *ctx,
> }
> }
>
> + /* don't clamp signed normalized as they have a different range */
> + if (_mesa_get_format_datatype(texFormat) == GL_SIGNED_NORMALIZED)
> + transferOps &= ~IMAGE_CLAMP_BIT;
> +
> /* If the format is unsigned normalized, we can ignore clamping
> * because the values are already in the range [0,1] so it won't
> * have any effect anyway.
> @@ -544,7 +548,9 @@ read_rgba_pixels( struct gl_context *ctx,
>
> /* Handle transfer ops if necessary */
> if (transferOps)
> - _mesa_apply_rgba_transfer_ops(ctx, transferOps, width * height,
> rgba);
> + _mesa_apply_rgba_transfer_ops(ctx, transferOps,
> + _mesa_get_format_datatype(rb_format)
> == GL_SIGNED_NORMALIZED,
Line too long. Not sure how to wrap that nicely, though.
> + width * height, rgba);
>
> /* If we had to rebase, we have already taken care of that */
> needs_rebase = false;
> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
> index fc3cc6b..c501f9e 100644
> --- a/src/mesa/main/texgetimage.c
> +++ b/src/mesa/main/texgetimage.c
> @@ -518,7 +518,9 @@ get_tex_rgba_uncompressed(struct gl_context *ctx, GLuint
> dimensions,
> needsRebase ? rebaseSwizzle : NULL);
>
> /* Handle transfer ops now */
> - _mesa_apply_rgba_transfer_ops(ctx, transferOps, width * height,
> rgba);
> + _mesa_apply_rgba_transfer_ops(ctx, transferOps,
> + _mesa_get_format_datatype(texFormat)
> == GL_SIGNED_NORMALIZED,
Same here.
> + width * height, rgba);
>
> /* If we had to rebase, we have already handled that */
> needsRebase = false;
> diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
> index b54e033..5131210 100644
> --- a/src/mesa/main/texstore.c
> +++ b/src/mesa/main/texstore.c
> @@ -779,7 +779,9 @@ texstore_rgba(TEXSTORE_PARAMS)
> }
>
> /* Apply transferOps */
> - _mesa_apply_rgba_transfer_ops(ctx, ctx->_ImageTransferState,
> elementCount,
> + _mesa_apply_rgba_transfer_ops(ctx, ctx->_ImageTransferState,
> + _mesa_get_format_datatype(srcMesaFormat)
> == GL_SIGNED_NORMALIZED,
Here
> + elementCount,
> (float(*)[4]) tempRGBA);
>
> /* Now we have to adjust our src info for a conversion from
> diff --git a/src/mesa/swrast/s_copypix.c b/src/mesa/swrast/s_copypix.c
> index 0dbccc0..c3cf7a3 100644
> --- a/src/mesa/swrast/s_copypix.c
> +++ b/src/mesa/swrast/s_copypix.c
> @@ -165,7 +165,9 @@ copy_rgba_pixels(struct gl_context *ctx, GLint srcx,
> GLint srcy,
> }
>
> if (transferOps) {
> - _mesa_apply_rgba_transfer_ops(ctx, transferOps, width,
> + _mesa_apply_rgba_transfer_ops(ctx, transferOps,
> + false,
> + width,
> (GLfloat (*)[4]) rgba);
> }
>
> diff --git a/src/mesa/swrast/s_drawpix.c b/src/mesa/swrast/s_drawpix.c
> index f05528d..78d9771 100644
> --- a/src/mesa/swrast/s_drawpix.c
> +++ b/src/mesa/swrast/s_drawpix.c
> @@ -516,7 +516,7 @@ draw_rgba_pixels( struct gl_context *ctx, GLint x, GLint
> y,
> (void*)source, srcMesaFormat, srcStride,
> spanWidth, 1, NULL);
> if (transferOps)
> - _mesa_apply_rgba_transfer_ops(ctx, transferOps, spanWidth,
> (GLfloat (*)[4])rgba);
> + _mesa_apply_rgba_transfer_ops(ctx, transferOps, false,
> spanWidth, (GLfloat (*)[4])rgba);
and here.
> /* Set these for each row since the _swrast_write_* functions
> * may change them while clipping/rendering.
> */
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
