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: _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
