[Mesa-dev] [PATCH 1/2] st/mesa: adjust blending modes if we don't have destination alpha

[Brian Paul]

On 04/28/2015 05:52 PM, Ilia Mirkin wrote:
> On Tue, Apr 28, 2015 at 7:52 PM, Brian Paul <brianp at vmware.com> wrote:
>> On 04/28/2015 04:35 PM, Ilia Mirkin wrote:
>>>
>>> On Tue, Apr 28, 2015 at 6:26 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>
>>>> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>
>>>
>>> aaaactually....
>>>
>>>>
>>>> Awesome! Now I can go remove this set of hacks from freedreno. And
>>>> this fixes the same issue in nouveau. Thanks for doing it the real way
>>>> :)
>>>>
>>>> On Tue, Apr 28, 2015 at 6:16 PM, Brian Paul <brianp at vmware.com> wrote:
>>>>>
>>>>> If the user requested a GL_RGB texture but the driver actually allocated
>>>>> an RGBA texture, the alpha values in the texture may not be defined.
>>>>>
>>>>> If we later bind the texture as a color target and try to blend into
>>>>> it with GL_DST_ALPHA or GL_ONE_MINUS_DST_ALPHA we may blend with
>>>>> undefined alpha values when, in fact, the dest alpha value should be
>>>>> one.
>>>>> So replace GL_DST_ALPHA/GL_ONE_MINUS_DST_ALPHA with GL_ONE/GL_ZERO.
>>>>>
>>>>> Fixes the piglit fbo-blending-formats test for some GL_RGB formats
>>>>> with the VMware driver.  Also tested with llvmpipe.
>>>>> ---
>>>>>    src/mesa/state_tracker/st_atom_blend.c | 38
>>>>> +++++++++++++++++++++++++---------
>>>>>    1 file changed, 28 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/state_tracker/st_atom_blend.c
>>>>> b/src/mesa/state_tracker/st_atom_blend.c
>>>>> index 6bb4077..30bff7a 100644
>>>>> --- a/src/mesa/state_tracker/st_atom_blend.c
>>>>> +++ b/src/mesa/state_tracker/st_atom_blend.c
>>>>> @@ -44,10 +44,21 @@
>>>>>    /**
>>>>>     * Convert GLenum blend tokens to pipe tokens.
>>>>>     * Both blend factors and blend funcs are accepted.
>>>>> + * \param destBaseFormat  the base format of the render target, such as
>>>>> + *                        GL_RGBA, GL_RGB, GL_RED, GL_ALPHA, etc.
>>>>>     */
>>>>>    static GLuint
>>>>> -translate_blend(GLenum blend)
>>>>> +translate_blend(GLenum blend, GLenum destBaseFormat)
>>>>>    {
>>>>> +   /* If we don't have destination alpha and the blend factor is either
>>>>> +    * GL_DST_ALPHA or GL_ONE_MINUS_DST_ALPHA then we use
>>>>> +    * PIPE_BLENDFACTOR_ONE or _ZERO instead.
>>>>> +    */
>>>>> +   const bool haveDstA = (destBaseFormat == GL_RGBA ||
>>>>> +                          destBaseFormat == GL_ALPHA ||
>>>>> +                          destBaseFormat == GL_INTENSITY ||
>>>>> +                          destBaseFormat == GL_LUMINANCE_ALPHA);
>>>>> +
>>>>>       switch (blend) {
>>>>>       /* blend functions */
>>>>>       case GL_FUNC_ADD:
>>>>> @@ -69,7 +80,7 @@ translate_blend(GLenum blend)
>>>>>       case GL_SRC_ALPHA:
>>>>>          return PIPE_BLENDFACTOR_SRC_ALPHA;
>>>>>       case GL_DST_ALPHA:
>>>>> -      return PIPE_BLENDFACTOR_DST_ALPHA;
>>>>> +      return haveDstA ? PIPE_BLENDFACTOR_DST_ALPHA :
>>>>> PIPE_BLENDFACTOR_ONE;
>>>>>       case GL_DST_COLOR:
>>>>>          return PIPE_BLENDFACTOR_DST_COLOR;
>>>>>       case GL_SRC_ALPHA_SATURATE:
>>>>> @@ -91,7 +102,7 @@ translate_blend(GLenum blend)
>>>>>       case GL_ONE_MINUS_DST_COLOR:
>>>>>          return PIPE_BLENDFACTOR_INV_DST_COLOR;
>>>>>       case GL_ONE_MINUS_DST_ALPHA:
>>>>> -      return PIPE_BLENDFACTOR_INV_DST_ALPHA;
>>>>> +      return haveDstA ? PIPE_BLENDFACTOR_INV_DST_ALPHA :
>>>>> PIPE_BLENDFACTOR_ZERO;
>>>>>       case GL_ONE_MINUS_CONSTANT_COLOR:
>>>>>          return PIPE_BLENDFACTOR_INV_CONST_COLOR;
>>>>>       case GL_ONE_MINUS_CONSTANT_ALPHA:
>>>>> @@ -208,14 +219,21 @@ update_blend( struct st_context *st )
>>>>>       else if (ctx->Color.BlendEnabled) {
>>>>>          /* blending enabled */
>>>>>          for (i = 0, j = 0; i < num_state; i++) {
>>>>> +         const struct gl_renderbuffer *rb;
>>>>> +         GLenum baseFormat;
>>>>>
>>>>>             blend->rt[i].blend_enable = (ctx->Color.BlendEnabled >> i) &
>>>>> 0x1;
>>>>>
>>>>>             if (ctx->Extensions.ARB_draw_buffers_blend)
>>>>>                j = i;
>>>>>
>>>>> +         /* _NEW_BUFFERS */
>>>>> +         /* Get the base format of the render target */
>>>>> +         rb = ctx->DrawBuffer->_ColorDrawBuffers[j];
>>>
>>>
>>> That's the wrong render target, no? You need the i'th render target.
>>
>>
>> Ah, I think you're right.
>>
>> BTW, I think there's more i/j mix-ups in this code (independent of this
>> patch).  I'll send a separate patch for that after I check the specs.
>>
>>
>>> And what happens if I'm not using independent blend but one of the
>>> RT's is RGB while the other is RGBA?
>>
>>
>> Yeah, there's no simple fix for that, AFAIK.
>
> Well, presumably if the driver supports independent blend, you could
> turn that on? If the driver doesn't do independent blend, then you're
> SOL, but you should at least leave the DST_ALPHA setting alone...

If we have independent blend, the patch takes care of this.  I assumed 
you were asking about the RGBA+RGB case if we don't have independent 
blend.  I guess I'm not too concerned about that for now.  Off-hand, I 
doubt we have any piglit tests that really exercise mixed buffer 
formats.  And I don't have time to write them myself right now.

-Brian