On 19/01/17 05:33, Iago Toral wrote: > 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?
Well, when I was working on this patch, I also tried that alternative, so as it keep failing I though that I was doing something wrong. In any case, while writing your answer I realized that perhaps the error is on the test that regresses (see below). > 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; Doing this we get a regression on the following test: ES3-CTS.functional.state_query.fbo.read_framebuffer_default_framebuffer That is failing on the following case: * framebuffer is default framebuffer * attachment is GL_DEPTH (also with GL_STENCIL) * att->Type is GL_RENDERBUFFER. The test expect default framebuffer, but it gets RENDERBUFFER. But I don't see anything on the spec forcing to return DEFAULT_FRAMEBUFFER for that case. > If this fixes the problem, I'd go with this version instead, as I find > it a lot easier to understand. I agree that is easier to understand. I will investigate a little more the test, to check if the regression is bogus. > >> ? 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 mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
