[Mesa-dev] [PATCH] i965: Avoid blending with destination alpha when RB format has no alpha bits

[Ian Romanick]

On 01/11/2013 07:27 AM, Carl Worth wrote:
> The hardware does not support a render target without an alpha channel.
> So when the user creates a render buffer with no alpha channel, there actually
> is storage available for alpha internally. It requires special care to
> avoid these unwanted alpha bits from causing any problems.
>
> Specifically, when blending, and when the blend factors would read the
> destination alpha values, this commit coerces the blend factors to instead be
> either 0 or 1 as appropriate.
>
> This commit fixes one portion of the following es3conform test:
>
> 	rgb8_rgba8_rgb
>
> (The test still fails due to a similar problem when copying from the
> renderbuffer to a texture.)

Curses!  I already fixed this for pre-GEN6... over 3 years ago! :(  See 
commit eadd9b8e from December 2009.  At the very least, I think your 
patch should use my fix_xRGB_alpha function since you're missing 
GL_SRC_ALPHA_SATURATE.  There may be other refactoring that could 
happen.  I see some duplication of the GL_MIN / GL_MAX checking, etc.

> ---
>
> Before pushing this, I plan to write a small piglit test demonstrating the
> actual bug being fixed here.
>
> As mentioned above, there's another problem with copying from a render-buffer
> to a texture. I plan to address that in a subsequent commit.
>
> I did think to see if there were other similar problems with blend
> factors. Ken helpes convince me that there should not be any problems with
> SRC_ALPHA blend factors since we don't have the same limitations on texture
> formats, (so we never need to be lying about alpha channels there).
>
> Also, I am not touching up the srcA or dstA blend factors even when they can
> involve destination alpha values, (such as DST_COLOR, DST_ALPHA, and their
> ONE_MINUS variants). This is because it doesn't make any sense to try to
> ensure ''correct'' values for the destination alpha when the whole point here
> is to ensure that those values can never change the results of any rendering.
>
>   src/mesa/drivers/dri/i965/gen6_cc.c |   28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/gen6_cc.c b/src/mesa/drivers/dri/i965/gen6_cc.c
> index b61a816c..776a11d 100644
> --- a/src/mesa/drivers/dri/i965/gen6_cc.c
> +++ b/src/mesa/drivers/dri/i965/gen6_cc.c
> @@ -118,6 +118,34 @@ gen6_upload_blend_state(struct brw_context *brw)
>   	    srcA = dstA = GL_ONE;
>   	 }
>
> +         /* Due to hardware limitations, the destination may have information
> +          * in an alpha channel even when the format specifies no alpha
> +          * channel. In order to avoid getting any incorrect blending due to
> +          * that alpha channel, coerce the blend factors to values that will
> +          * not read the alpha channel, but will instead use the correct
> +          * implicit value for alpha.
> +          */
> +         if (_mesa_get_format_bits(rb->Format, GL_ALPHA_BITS) == 0)
> +         {
> +            switch (srcRGB) {
> +            case GL_DST_ALPHA:
> +               srcRGB = GL_ONE;
> +               break;
> +            case GL_ONE_MINUS_DST_ALPHA:
> +               srcRGB = GL_ZERO;
> +               break;
> +            }
> +
> +            switch (dstRGB) {
> +            case GL_DST_ALPHA:
> +               dstRGB = GL_ONE;
> +               break;
> +            case GL_ONE_MINUS_DST_ALPHA:
> +               dstRGB = GL_ZERO;
> +               break;
> +            }
> +         }
> +
>   	 blend[b].blend0.dest_blend_factor = brw_translate_blend_factor(dstRGB);
>   	 blend[b].blend0.source_blend_factor = brw_translate_blend_factor(srcRGB);
>   	 blend[b].blend0.blend_func = brw_translate_blend_equation(eqRGB);
>