[Mesa-dev] [PATCH] mesa: fix error handling in get_framebuffer_parameteriv

[Tapani Pälli]

Hi;

On 05/05/2018 01:27 AM, Rhys Perry wrote:
> CC: <mesa-stable at lists.freedesktop.org>

These changes look correct to me and it looks cleaner with the common 
validate function. Do you know some app or test that got fixed by these 
changes?

> Signed-off-by: Rhys Perry <pendingchaos02 at gmail.com>
> ---
>   src/mesa/main/fbobject.c | 72 +++++++++++++++++++++++++++---------------------
>   1 file changed, 41 insertions(+), 31 deletions(-)
> 
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index c72204e11a..0fc3319fe9 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1488,45 +1488,66 @@ _mesa_FramebufferParameteri(GLenum target, GLenum pname, GLint param)
>   }
>   
>   static bool
> -_pname_valid_for_default_framebuffer(struct gl_context *ctx,
> -                                     GLenum pname)
> +validate_get_framebuffer_parameteriv_pname(struct gl_context *ctx,
> +                                           struct gl_framebuffer *fb,
> +                                           GLuint pname, const char *func)
>   {
> -   if (!_mesa_is_desktop_gl(ctx))
> -      return false;
> +   bool cannotBeWinsysFbo = true;

This is more like 'shouldn't be' right? I'm not sure how to make the 
variable name better though ...

Reviewed-by: Tapani Pälli <tapani.palli at intel.com>

>   
>      switch (pname) {
> +   case GL_FRAMEBUFFER_DEFAULT_LAYERS:
> +      /*
> +       * According to the OpenGL ES 3.1 specification section 9.2.3, the
> +       * GL_FRAMEBUFFER_LAYERS parameter name is not supported.
> +       */
> +      if (_mesa_is_gles31(ctx) && !ctx->Extensions.OES_geometry_shader) {
> +         _mesa_error(ctx, GL_INVALID_ENUM, "%s(pname=0x%x)", func, pname);
> +         return false;
> +      }
> +      break;
> +   case GL_FRAMEBUFFER_DEFAULT_WIDTH:
> +   case GL_FRAMEBUFFER_DEFAULT_HEIGHT:
> +   case GL_FRAMEBUFFER_DEFAULT_SAMPLES:
> +   case GL_FRAMEBUFFER_DEFAULT_FIXED_SAMPLE_LOCATIONS:
> +      break;
>      case GL_DOUBLEBUFFER:
>      case GL_IMPLEMENTATION_COLOR_READ_FORMAT:
>      case GL_IMPLEMENTATION_COLOR_READ_TYPE:
>      case GL_SAMPLES:
>      case GL_SAMPLE_BUFFERS:
>      case GL_STEREO:
> -      return true;
> +      /* From OpenGL 4.5 spec, section 9.2.3 "Framebuffer Object Queries:
> +       *
> +       *    "An INVALID_OPERATION error is generated by GetFramebufferParameteriv
> +       *     if the default framebuffer is bound to target and pname is not one
> +       *     of the accepted values from table 23.73, other than
> +       *     SAMPLE_POSITION."
> +       *
> +       * For OpenGL ES, using default framebuffer raises INVALID_OPERATION
> +       * for any pname.
> +       */
> +      cannotBeWinsysFbo = !_mesa_is_desktop_gl(ctx);
> +      break;
>      default:
> +      _mesa_error(ctx, GL_INVALID_ENUM, "%s(pname=0x%x)", func, pname);
>         return false;
>      }
> +
> +   if (cannotBeWinsysFbo && _mesa_is_winsys_fbo(fb)) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> +                  "%s(invalid pname=0x%x for default framebuffer)", func, pname);
> +      return false;
> +   }
> +
> +   return true;
>   }
>   
>   static void
>   get_framebuffer_parameteriv(struct gl_context *ctx, struct gl_framebuffer *fb,
>                               GLenum pname, GLint *params, const char *func)
>   {
> -   /* From OpenGL 4.5 spec, section 9.2.3 "Framebuffer Object Queries:
> -    *
> -    *    "An INVALID_OPERATION error is generated by GetFramebufferParameteriv
> -    *     if the default framebuffer is bound to target and pname is not one
> -    *     of the accepted values from table 23.73, other than
> -    *     SAMPLE_POSITION."
> -    *
> -    * For OpenGL ES, using default framebuffer still raises INVALID_OPERATION
> -    * for any pname.
> -    */
> -   if (_mesa_is_winsys_fbo(fb) &&
> -       !_pname_valid_for_default_framebuffer(ctx, pname)) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION,
> -                  "%s(invalid pname=0x%x for default framebuffer)", func, pname);
> +   if (!validate_get_framebuffer_parameteriv_pname(ctx, fb, pname, func))
>         return;
> -   }
>   
>      switch (pname) {
>      case GL_FRAMEBUFFER_DEFAULT_WIDTH:
> @@ -1536,14 +1557,6 @@ get_framebuffer_parameteriv(struct gl_context *ctx, struct gl_framebuffer *fb,
>         *params = fb->DefaultGeometry.Height;
>         break;
>      case GL_FRAMEBUFFER_DEFAULT_LAYERS:
> -      /*
> -       * According to the OpenGL ES 3.1 specification section 9.2.3, the
> -       * GL_FRAMEBUFFER_LAYERS parameter name is not supported.
> -       */
> -      if (_mesa_is_gles31(ctx) && !ctx->Extensions.OES_geometry_shader) {
> -         _mesa_error(ctx, GL_INVALID_ENUM, "%s(pname=0x%x)", func, pname);
> -         break;
> -      }
>         *params = fb->DefaultGeometry.Layers;
>         break;
>      case GL_FRAMEBUFFER_DEFAULT_SAMPLES:
> @@ -1570,9 +1583,6 @@ get_framebuffer_parameteriv(struct gl_context *ctx, struct gl_framebuffer *fb,
>      case GL_STEREO:
>         *params = fb->Visual.stereoMode;
>         break;
> -   default:
> -      _mesa_error(ctx, GL_INVALID_ENUM,
> -                  "%s(pname=0x%x)", func, pname);
>      }
>   }
>   
>