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

[Alejandro Piñeiro]

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)))
            ? 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:
-- 
2.9.3