[Mesa-dev] [PATCH v3] mesa/main: Fix FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE for NONE attachment type

[Iago Toral]

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

On Thu, 2017-01-19 at 11:36 -0200, Alejandro Piñeiro wrote:
> When the attachment type is NONE (att->Type),
> FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE should be NONE always.
> 
> Note that technically, the current behaviour follows the spec. From
> OpenGL 4.5 spec, Section 9.2.3 "Framebuffer Object Queries":
> 
>    "If the value of FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE is NONE, then
>     either no framebuffer is bound to target; or the default
>     framebuffer is bound, attachment is DEPTH or STENCIL, and the
>     number of depth or stencil bits, respectively, is zero."
> 
> Reading literally this paragraph, for the default framebuffer, NONE
> should be only returned if attachment is DEPTH and STENCIL without
> being allocated.
> 
> But it doesn't makes too much sense to return DEFAULT_FRAMEBUFFER if
> the attachment type is NONE. For example, this can happens if the
> attachment is FRONT_RIGHT run on monoscopic mode, as that attachment
> is only available on stereo mode.
> 
> With the current behaviour, defensive querying of the object type
> would not work properly. So you could query the object type checking
> for NONE, get DEFAULT_FRAMEBUFFER, and then get and INVALID_OPERATION
> when requesting other pnames (like RED_SIZE), as the real attachment
> type is NONE.
> 
> This fixes:
> GL45-CTS.direct_state_access.framebuffers_get_attachment_parameters
> 
> v2: don't change the behaviour for att->Type != GL_NONE, as caused
>     some ES CTS regressions
> v3: simplify condition (Iago)
> ---
> 
> Simplified version based on our IRC chatting. Fixes the test without
> any regression detected. Also expand the comment with the spec
> quote to justify the lack of explicit DEPTH or STENCIL checks.
> 
>  src/mesa/main/fbobject.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 044bd63..6934805 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -3754,11 +3754,13 @@
> _mesa_get_framebuffer_attachment_parameter(struct gl_context *ctx,
>         *  either no framebuffer is bound to target; or the default
> framebuffer
>         *  is bound, attachment is DEPTH or STENCIL, and the number
> of depth or
>         *  stencil bits, respectively, is zero."
> +       *
> +       * Note that we don't need explicit checks on DEPTH and
> STENCIL, because
> +       * on the case the spec is pointing, att->Type is already
> NONE, so we
> +       * just need to check att->Type.
>         */
> -      *params = (_mesa_is_winsys_fbo(buffer) &&
> -                 ((attachment != GL_DEPTH && attachment !=
> GL_STENCIL) ||
> -                  (att->Type != GL_NONE)))
> -         ? GL_FRAMEBUFFER_DEFAULT : att->Type;
> +      *params = (_mesa_is_winsys_fbo(buffer) && att->Type !=
> GL_NONE) ?
> +         GL_FRAMEBUFFER_DEFAULT : att->Type;
>        return;
>     case GL_FRAMEBUFFER_ATTACHMENT_OBJECT_NAME_EXT:
>        if (att->Type == GL_RENDERBUFFER_EXT) {