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

[Alejandro Piñeiro]

On 16/01/17 05:13, Iago Toral wrote:
> On Fri, 2017-01-13 at 12:15 -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.
> Makes sense to me, assuming this is not causing regressions anywhere:

I will make a full CI check before pushing, just in case.

> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Thanks!

>
> That said, we should file a bug to Khronos so they fix the text
> accordingly.

Yes, that was my idea. I wanted first a second opinion.

>> 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
>> ---
>>
>> CCing Iago Toral as he implemented the current OBJECT_TYPE compute
>> on commit cf4399.
>>
>> FWIW, that commit mentions the following test:
>> dEQP-
>> GLES3.functional.state_query.fbo.framebuffer_attachment_x_size_initia
>> l
>>
>> And I confirmed that the test doesn't regress with this change.
>>
>>  src/mesa/main/fbobject.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>> index 044bd63..7c1b035 100644
>> --- a/src/mesa/main/fbobject.c
>> +++ b/src/mesa/main/fbobject.c
>> @@ -3756,7 +3756,7 @@
>> _mesa_get_framebuffer_attachment_parameter(struct gl_context *ctx,
>>         *  stencil bits, respectively, is zero."
>>         */
>>        *params = (_mesa_is_winsys_fbo(buffer) &&
>> -                 ((attachment != GL_DEPTH && attachment !=
>> GL_STENCIL) ||
>> +                 ((attachment != GL_DEPTH && attachment !=
>> GL_STENCIL) &&
>>                    (att->Type != GL_NONE)))
>>           ? GL_FRAMEBUFFER_DEFAULT : att->Type;
>>        return;