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

[Iago Toral]

On Wed, 2017-01-18 at 16:48 -0200, Alejandro Piñeiro wrote:
> When the attachment type is NONE (att->Type),
> FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE should be NONE too.
> 
> 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 when att->Type != GL_NONE, as caused
>     some ES CTS regressions
> ---
> 
> The purpose of the patch was ensuring that OBJECT_TYPE is NONE if the
> saved attachment type (att->Type) is NONE. But v1 changed the
> behaviour on some cases, where att->Type != NONE, creating
> regressions
> on the following tests:
> 
> ES3-
> CTS.functional.state_query.fbo.read_framebuffer_default_framebuffer
>   ES3-
> CTS.functional.state_query.fbo.draw_framebuffer_default_framebuffer
> 
> v2 makes the condition somewhat harder to understand, and includes
> two
> atty->Type != GL_NONE checks. I was not able to find something more
> simple and shorter. A more simple and easy to read check would be:
> 
>       if (att->Type == GL_NONE)
>          *params = GL_NONE;
>       else
>          *params = (_mesa_is_winsys_fbo(buffer) &&
>                     ((attachment != GL_DEPTH && attachment !=
> GL_STENCIL) ||
>                      (att->Type != GL_NONE)))
                       ^^^^^^^^^^^^^^^^^^^^^^
Here we already know that att->Type != GL_NONE is true, so this part of
the condition does not add anything, right? In that case we could
simplify this to:

if (att->Type == GL_NONE)
   *params = GL_NONE;
else
   *params = _mesa_is_winsys_fbo(buffer) &&
              attachment != GL_DEPTH && attachment != GL_STENCIL
                 ? GL_FRAMEBUFFER_DEFAULT : att->Type;

If this fixes the problem, I'd go with this version instead, as I find
it a lot easier to understand.

>             ? GL_FRAMEBUFFER_DEFAULT : att->Type;
> 
> But seems somewhat verbose.
> 
> 
>  src/mesa/main/fbobject.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 044bd63..eee0b99 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -3757,7 +3757,8 @@
> _mesa_get_framebuffer_attachment_parameter(struct gl_context *ctx,
>         */
>        *params = (_mesa_is_winsys_fbo(buffer) &&
>                   ((attachment != GL_DEPTH && attachment !=
> GL_STENCIL) ||
> -                  (att->Type != GL_NONE)))
> +                  (att->Type != GL_NONE))) &&
> +         att->Type != GL_NONE
>           ? GL_FRAMEBUFFER_DEFAULT : att->Type;
>        return;
>     case GL_FRAMEBUFFER_ATTACHMENT_OBJECT_NAME_EXT: