[Mesa-dev] [PATCH 11/11] mesa: fix error value in GetFramebufferAttachmentParameteriv for OpenGL ES 3.0

[Ian Romanick]

On 01/20/2015 10:57 AM, Anuj Phogat wrote:
> On Mon, Jan 19, 2015 at 7:23 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 01/19/2015 03:32 AM, Eduardo Lima Mitev wrote:
>>> From: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>>
>>> Section 6.1.13 "Framebuffer Object Queries" of OpenGL ES 3.0 spec:
>>>
>>>  "If the default framebuffer is bound to target, then attachment must be
>>>   BACK, identifying the color buffer; DEPTH, identifying the depth buffer; or
>>>   STENCIL, identifying the stencil buffer."
>>>
>>> But the spec doesn't state which kind of error to return. GL_EXT_draw_buffers,
>>> section "Errors" says the following:
>>>
>>>   "The INVALID_ENUM error is generated by GetFramebufferAttachmentParameteriv
>>>    if the <attachment> parameter is not one of the values listed in Table 4.x."
>>>
>>> Due to the ambiguity of OpenGL ES 3.0 spec on this regard and because
>>> GL_EXT_draw_buffers returns INVALID_ENUM in a related case, then
>>> change the returned error to INVALID_ENUM.
>>
>> This is another section 2.3.1 (Errors) case:
>>
>>     "If a command that requires an enumerated value is passed a
>>     symbolic constant that is not one of those specified as allowable
>>     for that command, an INVALID_ENUM error is generated."
>>
>> The ES3.1 spec is also specific on this point:
>>
>>     An INVALID_ENUM error is generated by any combinations of
>>     framebuffer type and pname not described above.
>>
>> I'm not sure why we were generating INVALID_OPERATION previously.  This came
>> from
>>
>> commit 2f2801f876a4c637273bd3ddefb8a5b7a840e604
>> Author: Anuj Phogat <anuj.phogat at gmail.com>
>> Date:   Tue Dec 11 20:08:13 2012 -0800
>>
>>     mesa: Fix GL error generation in _mesa_GetFramebufferAttachmentParameteriv()
>>
>>     This allows query on default framebuffer in
>>     glGetFramebufferAttachmentParameteriv() for gles3. Fixes unexpected GL
>>     errors in gles3 conformance test case:
>>     framebuffer_blit_functionality_multisampled_to_singlesampled_blit
>>
>>     V2: Use _mesa_is_gles3() check to restrict allowed attachment types to
>>     specific APIs.
>>
>>     Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>     Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>>     Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>>
>> So... I wonder if framebuffer_blit_functionality_multisampled_to_singlesampled_blit
>> checks for INVALID_OPERATION here.  Either that or the error was just
>> copy-and-pasted from the previous case.  Anuj, do you recall?
>>
> framebuffer_blit_functionality_multisampled_to_singlesampled_blit continued
> passing if we change the error to GL_INVALID_ENUM. Seems like I copy
> pasted the gles 2.0 error from previous case :(.

Okay.  Thanks for the info.

This patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

>>> More info:
>>>
>>> http://lists.freedesktop.org/archives/mesa-dev/2015-January/074447.html
>>>
>>> Fixes:
>>>
>>> dEQP-GLES3.functional.fbo.api.attachment_query_default_fbo
>>>
>>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>> ---
>>>  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 80dc353..99ecee8 100644
>>> --- a/src/mesa/main/fbobject.c
>>> +++ b/src/mesa/main/fbobject.c
>>> @@ -2790,7 +2790,7 @@ _mesa_GetFramebufferAttachmentParameteriv(GLenum target, GLenum attachment,
>>>
>>>        if (_mesa_is_gles3(ctx) && attachment != GL_BACK &&
>>>            attachment != GL_DEPTH && attachment != GL_STENCIL) {
>>> -         _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +         _mesa_error(ctx, GL_INVALID_ENUM,
>>>                       "glGetFramebufferAttachmentParameteriv(attachment)");
>>>           return;
>>>        }
>>>
>>
>