[Mesa-dev] [PATCH 2/2] main/fbobject: throw invalid operation when get_attachment fails if needed

[Nicolai Hähnle]

On 12.01.2017 23:22, Alejandro Piñeiro wrote:
> In most cases, if a call to get_attachment fails is because attachment
> is a INVALID_ENUM. But for some specific cases, if COLOR_ATTACHMENTm
> (where m >= MAX_COLOR_ATTACHMENTS) is used, it should raise an
> INVALID_OPERATION exception instead.
>
> Fixes:
> GL45-CTS.direct_state_access.framebuffers_get_attachment_parameter_errors
> GL45-CTS.direct_state_access.framebuffers_renderbuffer_attachment_errors
> ---
>
> The code could be more simple if we do something like this:
>
>    err = is_color_attachment ? GL_INVALID_OPERATION : GL_INVALID_ENUM;
>    _mesa_error(...)
>
> Although that would mean having the same error message for both
> cases. Not a big deal. Im just slightly biased to differentiate a little.
>
>
>  src/mesa/main/fbobject.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index ce5eeae..871d08c 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -3495,6 +3495,7 @@ framebuffer_renderbuffer(struct gl_context *ctx,
>                           const char *func)
>  {
>     struct gl_renderbuffer_attachment *att;
> +   bool is_color_attachment;
>
>     if (_mesa_is_winsys_fbo(fb)) {
>        /* Can't attach new renderbuffers to a window system framebuffer */
> @@ -3503,11 +3504,28 @@ framebuffer_renderbuffer(struct gl_context *ctx,
>        return;
>     }
>
> -   att = get_attachment(ctx, fb, attachment, NULL);
> +   att = get_attachment(ctx, fb, attachment, &is_color_attachment);
>     if (att == NULL) {
> -      _mesa_error(ctx, GL_INVALID_ENUM,
> -                  "%s(invalid attachment %s)", func,
> -                  _mesa_enum_to_string(attachment));
> +      /*
> +       * From OpenGL 4.5 spec, section 9.2.7 "Attaching Renderbuffer Images to
> +       * a Framebuffer":
> +       *    "An INVALID_OPERATION error is generated if attachment is COLOR_-
> +       *     ATTACHMENTm where m is greater than or equal to the value of
> +       *     MAX_COLOR_- ATTACHMENTS ."

We usually have a new line before the quote block.

> +       *
> +       * If we are at this point, is because the attachment is not valid, so
> +       * if is_color_attachment is true, is because of the previous reason.
> +       */
> +      if (is_color_attachment) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION,
> +                     "%s(invalid attachment %s)", func,
> +                     _mesa_enum_to_string(attachment));

You probably want "invalid color attachment" here.

With these two comments addressed, the series is

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

> +      } else {
> +         _mesa_error(ctx, GL_INVALID_ENUM,
> +                     "%s(invalid attachment %s)", func,
> +                     _mesa_enum_to_string(attachment));
> +      }
> +
>        return;
>     }
>
> @@ -3609,6 +3627,7 @@ _mesa_get_framebuffer_attachment_parameter(struct gl_context *ctx,
>                                             GLint *params, const char *caller)
>  {
>     const struct gl_renderbuffer_attachment *att;
> +   bool is_color_attachment;
>     GLenum err;
>
>     /* The error code for an attachment type of GL_NONE differs between APIs.
> @@ -3676,12 +3695,27 @@ _mesa_get_framebuffer_attachment_parameter(struct gl_context *ctx,
>     }
>     else {
>        /* user-created framebuffer FBO */
> -      att = get_attachment(ctx, buffer, attachment, NULL);
> +      att = get_attachment(ctx, buffer, attachment, &is_color_attachment);
>     }
>
>     if (att == NULL) {
> -      _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid attachment %s)", caller,
> -                  _mesa_enum_to_string(attachment));
> +      /*
> +       * From OpenGL 4.5 spec, section 9.2.3 "Framebuffer Object Queries":
> +       *
> +       *    "An INVALID_OPERATION error is generated if a framebuffer object
> +       *     is bound to target and attachment is COLOR_ATTACHMENTm where m is
> +       *     greater than or equal to the value of MAX_COLOR_ATTACHMENTS."
> +       *
> +       * If we are at this point, is because the attachment is not valid, so
> +       * if is_color_attachment is true, is because of the previous reason.
> +       */
> +      if (is_color_attachment) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION, "%s(invalid color attachment %s)",
> +                     caller, _mesa_enum_to_string(attachment));
> +      } else {
> +         _mesa_error(ctx, GL_INVALID_ENUM, "%s(invalid attachment %s)", caller,
> +                     _mesa_enum_to_string(attachment));
> +      }
>        return;
>     }
>
>