[Mesa-dev] [PATCH 1/2] st/mesa: adjust blending modes if we don't have destination alpha
Roland Scheidegger
sroland at vmware.com
Tue Apr 28 16:59:36 PDT 2015
Am 29.04.2015 um 01:52 schrieb Brian Paul:
> 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 could turn that into independent blend if the the driver supports
it. This would probably be a reason why drivers may be better suited to
handle this (if they don't support independent blend they may or may not
still be able to handle this correctly), though it's true this logic is
quite duplicated among all drivers in practice.
Anyway, this is probably a good idea though I suspect unfortunately we
can't get rid of the same code in llvmpipe due to other state trackers
(not entirely sure though).
Roland
>
>
>
>>
>> -ilia
>>
>>>> + baseFormat = rb ? rb->_BaseFormat : GL_RGBA;
>>>> +
>>>> blend->rt[i].rgb_func =
>>>> - translate_blend(ctx->Color.Blend[j].EquationRGB);
>>>> + translate_blend(ctx->Color.Blend[j].EquationRGB,
>>>> baseFormat);
>>>>
>>>> if (ctx->Color.Blend[i].EquationRGB == GL_MIN ||
>>>> ctx->Color.Blend[i].EquationRGB == GL_MAX) {
>>>> @@ -225,13 +243,13 @@ update_blend( struct st_context *st )
>>>> }
>>>> else {
>>>> blend->rt[i].rgb_src_factor =
>>>> - translate_blend(ctx->Color.Blend[j].SrcRGB);
>>>> + translate_blend(ctx->Color.Blend[j].SrcRGB,
>>>> baseFormat);
>>>> blend->rt[i].rgb_dst_factor =
>>>> - translate_blend(ctx->Color.Blend[j].DstRGB);
>>>> + translate_blend(ctx->Color.Blend[j].DstRGB,
>>>> baseFormat);
>>>> }
>>>>
>>>> blend->rt[i].alpha_func =
>>>> - translate_blend(ctx->Color.Blend[j].EquationA);
>>>> + translate_blend(ctx->Color.Blend[j].EquationA,
>>>> baseFormat);
>>>>
>>>> if (ctx->Color.Blend[i].EquationA == GL_MIN ||
>>>> ctx->Color.Blend[i].EquationA == GL_MAX) {
>>>> @@ -241,9 +259,9 @@ update_blend( struct st_context *st )
>>>> }
>>>> else {
>>>> blend->rt[i].alpha_src_factor =
>>>> - translate_blend(ctx->Color.Blend[j].SrcA);
>>>> + translate_blend(ctx->Color.Blend[j].SrcA, baseFormat);
>>>> blend->rt[i].alpha_dst_factor =
>>>> - translate_blend(ctx->Color.Blend[j].DstA);
>>>> + translate_blend(ctx->Color.Blend[j].DstA, baseFormat);
>>>> }
>>>> }
>>>> }
>>>> @@ -285,7 +303,7 @@ update_blend( struct st_context *st )
>>>> const struct st_tracked_state st_update_blend = {
>>>> "st_update_blend", /* name */
>>>> { /* dirty */
>>>> - (_NEW_COLOR | _NEW_MULTISAMPLE), /* XXX _NEW_BLEND someday?
>>>> */ /* mesa */
>>>> + (_NEW_BUFFERS | _NEW_COLOR | _NEW_MULTISAMPLE), /*
>>>> mesa */
>>>> 0, /* st */
>>>> },
>>>> update_blend, /* update */
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=AwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=1W-JLrlfYq_4nAQK3JbzcEf5Kqlrrh6To8ER_0v6EB8&s=_FAPUJ8-6_fZ_dHkR-01c2z5riTWBgn2gfIxaN-nMXo&e=
>>>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Vjtt0vs_iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I&m=RevpopeMGsb_QQGQvHEY6dYMuA4GYREsK-agI-ip0p4&s=Nh8w2nPNnrjObvM05f1UsDuK9tD2FRt-JtjuNDaUw9I&e=
>
More information about the mesa-dev
mailing list