[Mesa-dev] [PATCH] llvmpipe: Fix alpha testing precision on rgba8 formats.

[Brian Paul]

On 05/22/2012 09:07 AM, jfonseca at vmware.com wrote:
> From: José Fonseca<jfonseca at vmware.com>
>
> This is a long standing problem, that recently surfaced with the change
> to enable perspective correct color interpolation.
>
> A fix for all possible formats is left to the future.
> ---
>   src/gallium/drivers/llvmpipe/lp_bld_alpha.c |   31 +++++++++++++++++++++++++++
>   src/gallium/drivers/llvmpipe/lp_bld_alpha.h |    2 ++
>   src/gallium/drivers/llvmpipe/lp_state_fs.c  |    5 ++++-
>   3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_bld_alpha.c b/src/gallium/drivers/llvmpipe/lp_bld_alpha.c
> index 518969c..bf4e8c4 100644
> --- a/src/gallium/drivers/llvmpipe/lp_bld_alpha.c
> +++ b/src/gallium/drivers/llvmpipe/lp_bld_alpha.c
> @@ -32,9 +32,12 @@
>    */
>
>   #include "pipe/p_state.h"
> +#include "util/u_format.h"
>
>   #include "gallivm/lp_bld_type.h"
>   #include "gallivm/lp_bld_const.h"
> +#include "gallivm/lp_bld_arit.h"
> +#include "gallivm/lp_bld_conv.h"
>   #include "gallivm/lp_bld_logic.h"
>   #include "gallivm/lp_bld_flow.h"
>   #include "gallivm/lp_bld_debug.h"
> @@ -46,6 +49,7 @@ void
>   lp_build_alpha_test(struct gallivm_state *gallivm,
>                       unsigned func,
>                       struct lp_type type,
> +                    const struct util_format_description *cbuf_format_desc,
>                       struct lp_build_mask_context *mask,
>                       LLVMValueRef alpha,
>                       LLVMValueRef ref,
> @@ -56,6 +60,33 @@ lp_build_alpha_test(struct gallivm_state *gallivm,
>
>      lp_build_context_init(&bld, gallivm, type);
>
> +   /*
> +    * Alpha testing needs to be done in the color buffer precision.
> +    *
> +    * TODO: Ideally, instead of duplicating the color conversion code, we would do
> +    * alpha testing after converting the output colors, but that's not very
> +    * convenient, because it needs to be done before depth testing.  Hopefully
> +    * LLVM will detect and remove the duplicate expression.
> +    *
> +    * FIXME: This should be generalized to formats other than rgba8 variants.
> +    * It is, however, difficult to come up with a truly generic rule that would
> +    * work for an arbitrary format.  For example, how many bits should we use
> +    * when doing alpha testing for B5G5R5X1_UNORM?
 > +    */

The GL spec addresses this in section 2.14.9 on page 71 of the 2.1 spec:

"""
For an RGBA color, each color component (which lies in [0, 1]) is 
converted (by rounding to nearest) to a fixed-point value with m bits.
[...]
m must be at least as large as the number of bits in the corresponding
component of the framebuffer. m must be at least 2 for A if the 
framebuffer does not contain an A component, or if there is only 1 bit 
of A in the framebuffer.
"""


> +   if (type.floating&&
> +       util_format_is_rgba8_variant(cbuf_format_desc)) {
> +      const unsigned dst_width = 8;
> +
> +      alpha = lp_build_clamp(&bld, alpha, bld.zero, bld.one);
> +      ref   = lp_build_clamp(&bld, ref,   bld.zero, bld.one);
> +
> +      alpha = lp_build_clamped_float_to_unsigned_norm(gallivm, type, dst_width, alpha);
> +      ref   = lp_build_clamped_float_to_unsigned_norm(gallivm, type, dst_width, ref);
> +
> +      type.floating = 0;
> +      lp_build_context_init(&bld, gallivm, type);
> +   }
> +
>      test = lp_build_cmp(&bld, func, alpha, ref);
>
>      lp_build_name(test, "alpha_mask");
> diff --git a/src/gallium/drivers/llvmpipe/lp_bld_alpha.h b/src/gallium/drivers/llvmpipe/lp_bld_alpha.h
> index 06206a2..15f1284 100644
> --- a/src/gallium/drivers/llvmpipe/lp_bld_alpha.h
> +++ b/src/gallium/drivers/llvmpipe/lp_bld_alpha.h
> @@ -39,6 +39,7 @@
>   #include "gallivm/lp_bld.h"
>
>   struct pipe_alpha_state;
> +struct util_format_description;
>   struct gallivm_state;
>   struct lp_type;
>   struct lp_build_mask_context;
> @@ -48,6 +49,7 @@ void
>   lp_build_alpha_test(struct gallivm_state *gallivm,
>                       unsigned func,
>                       struct lp_type type,
> +                    const struct util_format_description *cbuf_format_desc,
>                       struct lp_build_mask_context *mask,
>                       LLVMValueRef alpha,
>                       LLVMValueRef ref,
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index 11a3871..0bdc17f 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -345,13 +345,16 @@ generate_fs(struct gallivm_state *gallivm,
>                                              0);
>
>         if (color0 != -1&&  outputs[color0][3]) {
> +         const struct util_format_description *cbuf_format_desc;
>            LLVMValueRef alpha = LLVMBuildLoad(builder, outputs[color0][3], "alpha");
>            LLVMValueRef alpha_ref_value;
>
>            alpha_ref_value = lp_jit_context_alpha_ref_value(gallivm, context_ptr);
>            alpha_ref_value = lp_build_broadcast(gallivm, vec_type, alpha_ref_value);
>
> -         lp_build_alpha_test(gallivm, key->alpha.func, type,
> +         cbuf_format_desc = util_format_description(key->cbuf_format[0]);
> +
> +         lp_build_alpha_test(gallivm, key->alpha.func, type, cbuf_format_desc,
>                                &mask, alpha, alpha_ref_value,
>                                (depth_mode&  LATE_DEPTH_TEST) != 0);
>         }

Reviewed-by: Brian Paul <brianp at vmware.com>