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

[Alejandro Piñeiro]

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: