[1/3] mesa: rework error handling in glDrawBuffers

Submitted by Marek Olšák on April 29, 2019, 11:10 p.m.

Details

Message ID CAAxE2A5ew_uGTsLqfHcWk66vU9jxtC=qmtOnDs6HV2SYtJerqQ@mail.gmail.com
State New
Headers show
Series "Pbuffer fixes" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák April 29, 2019, 11:10 p.m.
I'm adding this hunk, which makes the test pass:

             return;

Marek

On Sat, Apr 27, 2019 at 10:23 AM Mathias Fröhlich <Mathias.Froehlich@gmx.net>
wrote:

> Hi Marek,
>
> one comment/failure inline:
>
> On Saturday, 27 April 2019 05:31:45 CEST Marek Olšák wrote:
> > From: Marek Olšák <marek.olsak@amd.com>
> >
> > It's needed by the next pbuffer fix, which changes the behavior of
> > draw_buffer_enum_to_bitmask, so it can't be used to help with error
> > checking.
> > ---
> >  src/mesa/main/buffers.c | 53 ++++++++++++++++++++++-------------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> > index d98c015bb24..2148fa1316c 100644
> > --- a/src/mesa/main/buffers.c
> > +++ b/src/mesa/main/buffers.c
> > @@ -430,65 +430,70 @@ draw_buffers(struct gl_context *ctx, struct
> gl_framebuffer *fb, GLsizei n,
> >           _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid buffers)",
> caller);
> >           return;
> >        }
> >     }
> >
> >     supportedMask = supported_buffer_bitmask(ctx, fb);
> >     usedBufferMask = 0x0;
> >
> >     /* complicated error checking... */
> >     for (output = 0; output < n; output++) {
> > -      destMask[output] = draw_buffer_enum_to_bitmask(ctx,
> buffers[output]);
> > -
> >        if (!no_error) {
> > -         /* From the OpenGL 3.0 specification, page 258:
> > -          * "Each buffer listed in bufs must be one of the values from
> tables
> > -          *  4.5 or 4.6.  Otherwise, an INVALID_ENUM error is generated.
> > -          */
> > -         if (destMask[output] == BAD_MASK) {
> > -            _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > -                        caller, _mesa_enum_to_string(buffers[output]));
> > -            return;
> > -         }
> > -
> >           /* From the OpenGL 4.5 specification, page 493 (page 515 of
> the PDF)
> >            * "An INVALID_ENUM error is generated if any value in bufs is
> FRONT,
> >            * LEFT, RIGHT, or FRONT_AND_BACK . This restriction applies
> to both
> >            * the default framebuffer and framebuffer objects, and exists
> because
> >            * these constants may themselves refer to multiple buffers,
> as shown
> >            * in table 17.4."
> >            *
> > -          * And on page 492 (page 514 of the PDF):
> > +          * From the OpenGL 4.5 specification, page 492 (page 514 of
> the PDF):
> >            * "If the default framebuffer is affected, then each of the
> constants
> >            * must be one of the values listed in table 17.6 or the
> special value
> >            * BACK. When BACK is used, n must be 1 and color values are
> written
> >            * into the left buffer for single-buffered contexts, or into
> the back
> >            * left buffer for double-buffered contexts."
> >            *
> >            * Note "special value BACK". GL_BACK also refers to multiple
> buffers,
> >            * but it is consider a special case here. This is a change on
> 4.5.
> >            * For OpenGL 4.x we check that behaviour. For any previous
> version we
> >            * keep considering it wrong (as INVALID_ENUM).
> >            */
> > -         if (util_bitcount(destMask[output]) > 1) {
> > -            if (_mesa_is_winsys_fbo(fb) && ctx->Version >= 40 &&
> > -                buffers[output] == GL_BACK) {
> > -               if (n != 1) {
> > -                  _mesa_error(ctx, GL_INVALID_OPERATION, "%s(with
> GL_BACK n must be 1)",
> > -                              caller);
> > -                  return;
> > -               }
> > -            } else {
> > -               _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer
> %s)",
> > -                           caller,
> _mesa_enum_to_string(buffers[output]));
> > +         if (buffers[output] == GL_BACK &&
> > +             _mesa_is_winsys_fbo(fb) &&
> > +             _mesa_is_desktop_gl(ctx) &&
> > +             ctx->Version >= 40) {
> > +            if (n != 1) {
> > +               _mesa_error(ctx, GL_INVALID_OPERATION, "%s(with GL_BACK
> n must be 1)",
> > +                           caller);
> >                 return;
> >              }
> > +         } else if (buffers[output] == GL_FRONT ||
> > +                    buffers[output] == GL_LEFT ||
> > +                    buffers[output] == GL_RIGHT ||
> > +                    buffers[output] == GL_FRONT_AND_BACK) {
> > +            _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > +                        caller, _mesa_enum_to_string(buffers[output]));
> > +            return;
> > +         }
> > +      }
>
> According to the comment above "Note "special value BACK". GL_BACK...",
> there must also be a GL_INVALID_ENUM on GL_BACK when not in opengles.
> Right?
>
> That specific case can be tested using
> MESA_GL_VERSION_OVERRIDE=3.3 piglit/bin/gl-3.1-draw-buffers-errors
>
> best
>
> Mathias
>
> > +
> > +      destMask[output] = draw_buffer_enum_to_bitmask(ctx,
> buffers[output]);
> > +
> > +      if (!no_error) {
> > +         /* From the OpenGL 3.0 specification, page 258:
> > +          * "Each buffer listed in bufs must be one of the values from
> tables
> > +          *  4.5 or 4.6.  Otherwise, an INVALID_ENUM error is generated.
> > +          */
> > +         if (destMask[output] == BAD_MASK) {
> > +            _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > +                        caller, _mesa_enum_to_string(buffers[output]));
> > +            return;
> >           }
> >
> >           /* Section 4.2 (Whole Framebuffer Operations) of the OpenGL ES
> 3.0
> >            * specification says:
> >            *
> >            *     "If the GL is bound to a draw framebuffer object, the
> ith
> >            *     buffer listed in bufs must be COLOR_ATTACHMENTi or NONE
> .
> >            *     Specifying a buffer out of order, BACK , or
> COLOR_ATTACHMENTm
> >            *     where m is greater than or equal to the value of MAX_-
> >            *     COLOR_ATTACHMENTS , will generate the error
> INVALID_OPERATION .
> >
>
>
>
>
>

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
index 1ac0d5d0798..a46599a2872 100644
--- a/src/mesa/main/buffers.c
+++ b/src/mesa/main/buffers.c
@@ -485,7 +485,9 @@  draw_buffers(struct gl_context *ctx, struct
gl_framebuffer *fb, GLsizei n,
          } else if (buffers[output] == GL_FRONT ||
                     buffers[output] == GL_LEFT ||
                     buffers[output] == GL_RIGHT ||
-                    buffers[output] == GL_FRONT_AND_BACK) {
+                    buffers[output] == GL_FRONT_AND_BACK ||
+                    (buffers[output] == GL_BACK &&
+                     _mesa_is_desktop_gl(ctx))) {
             _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
                         caller, _mesa_enum_to_string(buffers[output]));

Comments

Hi Marek,

On Tuesday, 30 April 2019 01:10:42 CEST Marek Olšák wrote:
> I'm adding this hunk, which makes the test pass:
> 
> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> index 1ac0d5d0798..a46599a2872 100644
> --- a/src/mesa/main/buffers.c
> +++ b/src/mesa/main/buffers.c
> @@ -485,7 +485,9 @@ draw_buffers(struct gl_context *ctx, struct
> gl_framebuffer *fb, GLsizei n,
>           } else if (buffers[output] == GL_FRONT ||
>                      buffers[output] == GL_LEFT ||
>                      buffers[output] == GL_RIGHT ||
> -                    buffers[output] == GL_FRONT_AND_BACK) {
> +                    buffers[output] == GL_FRONT_AND_BACK ||
> +                    (buffers[output] == GL_BACK &&
> +                     _mesa_is_desktop_gl(ctx))) {
>              _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
>                          caller, _mesa_enum_to_string(buffers[output]));
>              return;

If we finally go that route by this series then this patch looks good to me.
Lets see if we need a better overall fix for pbuffers.

best
Mathias

> 
> Marek
> 
> On Sat, Apr 27, 2019 at 10:23 AM Mathias Fröhlich <Mathias.Froehlich@gmx.net>
> wrote:
> 
> > Hi Marek,
> >
> > one comment/failure inline:
> >
> > On Saturday, 27 April 2019 05:31:45 CEST Marek Olšák wrote:
> > > From: Marek Olšák <marek.olsak@amd.com>
> > >
> > > It's needed by the next pbuffer fix, which changes the behavior of
> > > draw_buffer_enum_to_bitmask, so it can't be used to help with error
> > > checking.
> > > ---
> > >  src/mesa/main/buffers.c | 53 ++++++++++++++++++++++-------------------
> > >  1 file changed, 29 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> > > index d98c015bb24..2148fa1316c 100644
> > > --- a/src/mesa/main/buffers.c
> > > +++ b/src/mesa/main/buffers.c
> > > @@ -430,65 +430,70 @@ draw_buffers(struct gl_context *ctx, struct
> > gl_framebuffer *fb, GLsizei n,
> > >           _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid buffers)",
> > caller);
> > >           return;
> > >        }
> > >     }
> > >
> > >     supportedMask = supported_buffer_bitmask(ctx, fb);
> > >     usedBufferMask = 0x0;
> > >
> > >     /* complicated error checking... */
> > >     for (output = 0; output < n; output++) {
> > > -      destMask[output] = draw_buffer_enum_to_bitmask(ctx,
> > buffers[output]);
> > > -
> > >        if (!no_error) {
> > > -         /* From the OpenGL 3.0 specification, page 258:
> > > -          * "Each buffer listed in bufs must be one of the values from
> > tables
> > > -          *  4.5 or 4.6.  Otherwise, an INVALID_ENUM error is generated.
> > > -          */
> > > -         if (destMask[output] == BAD_MASK) {
> > > -            _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > > -                        caller, _mesa_enum_to_string(buffers[output]));
> > > -            return;
> > > -         }
> > > -
> > >           /* From the OpenGL 4.5 specification, page 493 (page 515 of
> > the PDF)
> > >            * "An INVALID_ENUM error is generated if any value in bufs is
> > FRONT,
> > >            * LEFT, RIGHT, or FRONT_AND_BACK . This restriction applies
> > to both
> > >            * the default framebuffer and framebuffer objects, and exists
> > because
> > >            * these constants may themselves refer to multiple buffers,
> > as shown
> > >            * in table 17.4."
> > >            *
> > > -          * And on page 492 (page 514 of the PDF):
> > > +          * From the OpenGL 4.5 specification, page 492 (page 514 of
> > the PDF):
> > >            * "If the default framebuffer is affected, then each of the
> > constants
> > >            * must be one of the values listed in table 17.6 or the
> > special value
> > >            * BACK. When BACK is used, n must be 1 and color values are
> > written
> > >            * into the left buffer for single-buffered contexts, or into
> > the back
> > >            * left buffer for double-buffered contexts."
> > >            *
> > >            * Note "special value BACK". GL_BACK also refers to multiple
> > buffers,
> > >            * but it is consider a special case here. This is a change on
> > 4.5.
> > >            * For OpenGL 4.x we check that behaviour. For any previous
> > version we
> > >            * keep considering it wrong (as INVALID_ENUM).
> > >            */
> > > -         if (util_bitcount(destMask[output]) > 1) {
> > > -            if (_mesa_is_winsys_fbo(fb) && ctx->Version >= 40 &&
> > > -                buffers[output] == GL_BACK) {
> > > -               if (n != 1) {
> > > -                  _mesa_error(ctx, GL_INVALID_OPERATION, "%s(with
> > GL_BACK n must be 1)",
> > > -                              caller);
> > > -                  return;
> > > -               }
> > > -            } else {
> > > -               _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer
> > %s)",
> > > -                           caller,
> > _mesa_enum_to_string(buffers[output]));
> > > +         if (buffers[output] == GL_BACK &&
> > > +             _mesa_is_winsys_fbo(fb) &&
> > > +             _mesa_is_desktop_gl(ctx) &&
> > > +             ctx->Version >= 40) {
> > > +            if (n != 1) {
> > > +               _mesa_error(ctx, GL_INVALID_OPERATION, "%s(with GL_BACK
> > n must be 1)",
> > > +                           caller);
> > >                 return;
> > >              }
> > > +         } else if (buffers[output] == GL_FRONT ||
> > > +                    buffers[output] == GL_LEFT ||
> > > +                    buffers[output] == GL_RIGHT ||
> > > +                    buffers[output] == GL_FRONT_AND_BACK) {
> > > +            _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > > +                        caller, _mesa_enum_to_string(buffers[output]));
> > > +            return;
> > > +         }
> > > +      }
> >
> > According to the comment above "Note "special value BACK". GL_BACK...",
> > there must also be a GL_INVALID_ENUM on GL_BACK when not in opengles.
> > Right?
> >
> > That specific case can be tested using
> > MESA_GL_VERSION_OVERRIDE=3.3 piglit/bin/gl-3.1-draw-buffers-errors
> >
> > best
> >
> > Mathias
> >
> > > +
> > > +      destMask[output] = draw_buffer_enum_to_bitmask(ctx,
> > buffers[output]);
> > > +
> > > +      if (!no_error) {
> > > +         /* From the OpenGL 3.0 specification, page 258:
> > > +          * "Each buffer listed in bufs must be one of the values from
> > tables
> > > +          *  4.5 or 4.6.  Otherwise, an INVALID_ENUM error is generated.
> > > +          */
> > > +         if (destMask[output] == BAD_MASK) {
> > > +            _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid buffer %s)",
> > > +                        caller, _mesa_enum_to_string(buffers[output]));
> > > +            return;
> > >           }
> > >
> > >           /* Section 4.2 (Whole Framebuffer Operations) of the OpenGL ES
> > 3.0
> > >            * specification says:
> > >            *
> > >            *     "If the GL is bound to a draw framebuffer object, the
> > ith
> > >            *     buffer listed in bufs must be COLOR_ATTACHMENTi or NONE
> > .
> > >            *     Specifying a buffer out of order, BACK , or
> > COLOR_ATTACHMENTm
> > >            *     where m is greater than or equal to the value of MAX_-
> > >            *     COLOR_ATTACHMENTS , will generate the error
> > INVALID_OPERATION .
> > >
> >
> >
> >
> >
> >
>