[Mesa-dev] [PATCH v2 10/16] mesa: Add draw time validation for advanced blending modes.

[Kenneth Graunke]

On Monday, August 22, 2016 1:39:54 PM PDT Francisco Jerez wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/main/context.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> > index 4ff0979..e63d8a6 100644
> > --- a/src/mesa/main/context.c
> > +++ b/src/mesa/main/context.c
> > @@ -1876,6 +1876,57 @@ check_blend_func_error(struct gl_context *ctx)
> >  	 return false;
> >        }
> >     }
> > +
> > +   if (ctx->Color.BlendEnabled && ctx->Color._AdvancedBlendMode) {
> > +      /* The KHR_blend_equation_advanced spec says:
> > +       *
> > +       *    "If any non-NONE draw buffer uses a blend equation found in table
> > +       *     X.1 or X.2, the error INVALID_OPERATION is generated by Begin or
> > +       *     any operation that implicitly calls Begin (such as DrawElements)
> > +       *     if:
> > +       *
> > +       *       * the draw buffer for color output zero selects multiple color
> > +       *         buffers (e.g., FRONT_AND_BACK in the default framebuffer); or
> > +       *
> > +       *       * the draw buffer for any other color output is not NONE."
> > +       */
> > +      if (ctx->DrawBuffer->ColorDrawBuffer[0] == GL_FRONT_AND_BACK) {
> > +         _mesa_error(ctx, GL_INVALID_OPERATION,
> > +                     "advanced blending is active and draw buffer for color "
> > +                     "output zero selects multiple color buffers");
> > +         return false;
> > +      }
> > +
> > +      for (unsigned i = 1; i < ctx->DrawBuffer->_NumColorDrawBuffers; i++) {
> > +         if (ctx->DrawBuffer->ColorDrawBuffer[i] != GL_NONE) {
> > +            _mesa_error(ctx, GL_INVALID_OPERATION,
> > +                        "advanced blending is active with multiple color "
> > +                        "draw buffers");
> > +            return false;
> > +         }
> > +      }
> > +
> > +      /* The KHR_blend_equation_advanced spec says:
> > +       *
> > +       *    "Advanced blending equations require the use of a fragment shader
> > +       *     with a matching "blend_support" layout qualifier.  If the current
> > +       *     blend equation is found in table X.1 or X.2, and the active
> > +       *     fragment shader does not include the layout qualifier matching
> > +       *     the blend equation or "blend_support_all_equations", the error
> > +       *     INVALID_OPERATION is generated [...]"
> > +       */
> > +      struct gl_shader_program *sh_prog = ctx->_Shader->_CurrentFragmentProgram;
> > +      struct gl_shader_info *fs_info =
> > +         &sh_prog->_LinkedShaders[MESA_SHADER_FRAGMENT]->info;
> 
> Don't you need a NULL check here to avoid a crash if
> there is no valid fragment program bound to the pipeline?  You could
> also declare the variables above as pointers to const.  With that fixed:
> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>

I think you're right.  I assumed that the caller would have checked that
before this point, but it's not obvious that it does without a lot of
chasing.  So...we may as well do it.

I've changed it to:

      const struct gl_shader_program *sh_prog =
         ctx->_Shader->_CurrentFragmentProgram;
      const GLbitfield blend_support = !sh_prog ? 0 :
         sh_prog->_LinkedShaders[MESA_SHADER_FRAGMENT]->info.BlendSupport;
 
      if ((blend_support & ctx->Color._AdvancedBlendMode) == 0) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160823/62ccdc16/attachment-0001.sig>